Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: use page visibilityState for browser responsiveness check #813

Merged
merged 3 commits into from
Jun 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions packages/rum-core/src/bootstrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,13 @@

import { isPlatformSupported, isBrowser } from './common/utils'
import { patchAll } from './common/patching'
import { bootstrapMetrics } from './performance-monitoring/metrics'
import { state } from './state'

let enabled = false
export function bootstrap() {
if (isPlatformSupported()) {
patchAll()
bootstrapMetrics()
bootstrapPerf()
enabled = true
} else if (isBrowser) {
/**
Expand All @@ -43,3 +43,19 @@ export function bootstrap() {

return enabled
}

export function bootstrapPerf() {
if (document.visibilityState === 'hidden') {
state.lastHiddenStart = 0
}

window.addEventListener(
'visibilitychange',
() => {
if (document.visibilityState === 'hidden') {
state.lastHiddenStart = performance.now()
}
},
{ capture: true }
)
}
2 changes: 1 addition & 1 deletion packages/rum-core/src/common/apm-server.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import {
compressTransaction,
compressError
} from './compress'
import { __DEV__ } from '../env'
import { __DEV__ } from '../state'

/**
* Throttling interval defaults to 60 seconds
Expand Down
6 changes: 0 additions & 6 deletions packages/rum-core/src/common/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -129,11 +129,6 @@ const FIRST_CONTENTFUL_PAINT = 'first-contentful-paint'
const LARGEST_CONTENTFUL_PAINT = 'largest-contentful-paint'
const FIRST_INPUT = 'first-input'

/**
* Managed transaction configs
*/
const BROWSER_RESPONSIVENESS_INTERVAL = 500

/**
* Event types sent to APM Server on the queue
*/
Expand Down Expand Up @@ -194,7 +189,6 @@ export {
TEMPORARY_TYPE,
USER_INTERACTION,
TRANSACTION_TYPE_ORDER,
BROWSER_RESPONSIVENESS_INTERVAL,
ERRORS,
TRANSACTIONS,
CONFIG_SERVICE,
Expand Down
2 changes: 1 addition & 1 deletion packages/rum-core/src/common/service-factory.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import {
LOGGING_SERVICE,
APM_SERVER
} from './constants'
import { __DEV__ } from '../env'
import { __DEV__ } from '../state'

const serviceCreators = {
[CONFIG_SERVICE]: () => new ConfigService(),
Expand Down
2 changes: 1 addition & 1 deletion packages/rum-core/src/opentracing/tracer.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import {
} from 'opentracing/lib/constants'
import { Span as NoopSpan } from 'opentracing/lib/span'
import { getTimeOrigin, find } from '../common/utils'
import { __DEV__ } from '../env'
import { __DEV__ } from '../state'
import Span from './span'

class Tracer extends otTracer {
Expand Down
16 changes: 1 addition & 15 deletions packages/rum-core/src/performance-monitoring/metrics.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ function createLongTaskSpans(longtasks) {
export function createFirstInputDelaySpan(fidEntries) {
let firstInput = fidEntries[0]

if (firstInput && !metrics.wasHidden) {
if (firstInput) {
hmdhk marked this conversation as resolved.
Show resolved Hide resolved
const { startTime, processingStart } = firstInput

const span = new Span('First Input Delay', FIRST_INPUT, { startTime })
Expand Down Expand Up @@ -292,17 +292,3 @@ export class PerfEntryRecorder {
this.po.disconnect()
}
}

export function bootstrapMetrics() {
metrics.wasHidden = document.visibilityState === 'hidden'

window.addEventListener(
'visibilitychange',
() => {
if (document.visibilityState === 'hidden') {
metrics.wasHidden = true
}
},
{ capture: true, once: true }
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,21 +43,18 @@ import {
XMLHTTPREQUEST,
EVENT_TARGET,
HTTP_REQUEST_TYPE,
USER_INTERACTION,
PAGE_LOAD,
BROWSER_RESPONSIVENESS_INTERVAL
USER_INTERACTION
} from '../common/constants'
import {
truncateModel,
SPAN_MODEL,
TRANSACTION_MODEL
} from '../common/truncate'
import { __DEV__ } from '../env'
import { __DEV__ } from '../state'

/**
* Parameters used for Managed Transactions
*/
const BROWSER_RESPONSIVENESS_BUFFER = 3
const SIMILAR_SPAN_TO_TRANSACTION_RATIO = 0.05
const TRANSACTION_DURATION_THRESHOLD = 60000

Expand Down Expand Up @@ -141,14 +138,6 @@ export function adjustTransactionSpans(transaction) {
return transaction
}

export function checkBrowserResponsiveness(transaction, interval, buffer) {
const counter = transaction.browserResponsivenessCounter
const duration = transaction.duration()
const expectedCount = Math.floor(duration / interval)

return counter + buffer >= expectedCount
}

export default class PerformanceMonitoring {
constructor(apmServer, configService, loggingService, transactionService) {
this._apmServer = apmServer
Expand Down Expand Up @@ -378,30 +367,6 @@ export default class PerformanceMonitoring {
}
return false
}

/**
* TODO: Refactor the type check with better logic
*/
if (tr.type !== PAGE_LOAD) {
const wasBrowserResponsive = checkBrowserResponsiveness(
tr,
BROWSER_RESPONSIVENESS_INTERVAL,
BROWSER_RESPONSIVENESS_BUFFER
)

if (!wasBrowserResponsive) {
if (__DEV__) {
this._logginService.debug(
`transaction(${tr.id}, ${tr.name}) was discarded! Browser was not responsive enough during the transaction.`,
' duration:',
duration,
' browserResponsivenessCounter:',
tr.browserResponsivenessCounter
)
}
return false
}
}
}
return true
}
Expand Down
39 changes: 11 additions & 28 deletions packages/rum-core/src/performance-monitoring/transaction-service.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import {
NAME_UNKNOWN,
TRANSACTION_START,
TRANSACTION_END,
BROWSER_RESPONSIVENESS_INTERVAL,
TEMPORARY_TYPE,
TRANSACTION_TYPE_ORDER,
LARGEST_CONTENTFUL_PAINT,
Expand All @@ -48,7 +47,7 @@ import {
FIRST_INPUT
} from '../common/constants'
import { addTransactionContext } from '../common/context'
import { __DEV__ } from '../env'
import { __DEV__, state } from '../state'

class TransactionService {
constructor(logger, config) {
Expand Down Expand Up @@ -91,28 +90,6 @@ class TransactionService {
this.currentTransaction = value
}

ensureRespInterval(checkBrowserResponsiveness) {
const clearRespInterval = () => {
clearInterval(this.respIntervalId)
this.respIntervalId = undefined
}

if (checkBrowserResponsiveness) {
if (typeof this.respIntervalId === 'undefined') {
this.respIntervalId = setInterval(() => {
let tr = this.getCurrentTransaction()
if (tr) {
tr.browserResponsivenessCounter++
} else {
clearRespInterval()
}
}, BROWSER_RESPONSIVENESS_INTERVAL)
}
} else if (typeof this.respIntervalId !== 'undefined') {
clearRespInterval()
}
}

createOptions(options) {
const config = this._config.config
let presetOptions = { transactionSampleRate: config.transactionSampleRate }
Expand Down Expand Up @@ -176,14 +153,12 @@ class TransactionService {
tr = this.ensureCurrentTransaction(name, type, perfOptions)
}

let checkBrowserResponsiveness = true
if (tr.type === PAGE_LOAD) {
if (!isRedefined) {
this.recorder.start(LARGEST_CONTENTFUL_PAINT)
this.recorder.start(PAINT)
this.recorder.start(FIRST_INPUT)
}
checkBrowserResponsiveness = false
if (perfOptions.pageLoadTraceId) {
tr.traceId = perfOptions.pageLoadTraceId
}
Expand Down Expand Up @@ -212,8 +187,6 @@ class TransactionService {
tr.captureTimings = true
}

this.ensureRespInterval(checkBrowserResponsiveness)

return tr
}

Expand Down Expand Up @@ -262,6 +235,16 @@ class TransactionService {
return Promise.resolve().then(
() => {
const { name, type } = tr
let { lastHiddenStart } = state

if (lastHiddenStart >= tr._start) {
if (__DEV__) {
this._logger.debug(
`transaction(${tr.id}, ${tr.name}, ${tr.type}) was discarded! The page was hidden during the transaction!`
)
}
return
}

if (this.shouldIgnoreTransaction(name) || type === TEMPORARY_TYPE) {
if (__DEV__) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ class Transaction extends SpanBase {
this.breakdownTimings = []

this.sampled = Math.random() <= this.options.transactionSampleRate
this.browserResponsivenessCounter = 0
}

addMarks(obj) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,5 @@
const __DEV__ = process.env.NODE_ENV !== 'production'

export { __DEV__ }

export const state = {}
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import Transaction from '../../src/performance-monitoring/transaction'
import Span from '../../src/performance-monitoring/span'
import {
groupSmallContinuouslySimilarSpans,
checkBrowserResponsiveness,
adjustTransactionSpans
} from '../../src/performance-monitoring/performance-monitoring'
import { getGlobalConfig } from '../../../../dev-utils/test-config'
Expand Down Expand Up @@ -121,31 +120,6 @@ describe('PerformanceMonitoring', function() {
)
})

it('should filter transactions based on browser responsiveness', function() {
spyOn(logger, 'debug').and.callThrough()
expect(logger.debug).not.toHaveBeenCalled()
var tr = new Transaction('transaction', 'transaction', {
id: 212,
transactionSampleRate: 1,
managed: true,
startTime: 1
})
var span = tr.startSpan('test span', 'test span type')
span.end()
tr.end(3501)

tr.browserResponsivenessCounter = 3
var wasBrowserResponsive = performanceMonitoring.filterTransaction(tr)
expect(wasBrowserResponsive).toBe(false)
expect(logger.debug).toHaveBeenCalledWith(
'transaction(212, transaction) was discarded! Browser was not responsive enough during the transaction.',
' duration:',
3500,
' browserResponsivenessCounter:',
3
)
})

it('should initialize and add transaction to the queue', async () => {
performanceMonitoring.init()
spyOn(apmServer, 'addTransaction')
Expand Down Expand Up @@ -816,32 +790,6 @@ describe('PerformanceMonitoring', function() {
expect(grouped[1].name).toBe('another-name')
})

it('should calculate browser responsiveness', function() {
const tr = new Transaction('transaction', 'transaction', {
startTime: 1
})
tr.end(400)

tr.browserResponsivenessCounter = 0
var resp = checkBrowserResponsiveness(tr, 500, 2)
expect(resp).toBe(true)

tr._end = 1001
tr.browserResponsivenessCounter = 2
resp = checkBrowserResponsiveness(tr, 500, 2)
expect(resp).toBe(true)

tr._end = 1601
tr.browserResponsivenessCounter = 2
resp = checkBrowserResponsiveness(tr, 500, 2)
expect(resp).toBe(true)

tr._end = 3001
tr.browserResponsivenessCounter = 3
resp = checkBrowserResponsiveness(tr, 500, 2)
expect(resp).toBe(false)
})

it('should reset spans for unsampled transactions', function() {
const tr = new Transaction('unsampled', 'test', {
transactionSampleRate: 0,
Expand Down
Loading