Skip to content

Commit

Permalink
fix: Revert "Generate PTID in Agent" (#976)
Browse files Browse the repository at this point in the history
  • Loading branch information
cwli24 authored Apr 15, 2024
1 parent c3e1e09 commit 34b317f
Show file tree
Hide file tree
Showing 8 changed files with 56 additions and 39 deletions.
2 changes: 1 addition & 1 deletion src/common/harvest/harvest.js
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ export class Harvest extends SharedContext {
result.addEventListener('loadend', function () {
// `this` refers to the XHR object in this scope, do not change this to a fat arrow
// status 0 refers to a local error, such as CORS or network failure, or a blocked request by the browser (e.g. adblocker)
const cbResult = { sent: this.status !== 0, status: this.status, failed: this.status === 0 || this.status >= 400, xhr: this, fullUrl }
const cbResult = { sent: this.status !== 0, status: this.status, xhr: this, fullUrl }
if (this.status === 429) {
cbResult.retry = true
cbResult.delay = harvestScope.tooManyRequestsDelay
Expand Down
34 changes: 16 additions & 18 deletions src/features/session_trace/aggregate/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,10 @@ export class Aggregate extends AggregateBase {
if (!this.agentRuntime.xhrWrappable) return

this.resourceObserver = argsObj?.resourceObserver // undefined if observer couldn't be created
this.ptid = this.agentRuntime.ptid
this.ptid = ''
this.trace = {}
this.nodeCount = 0
this.sentTrace = null
this.everSent = false
this.harvestTimeSeconds = getConfigurationValue(agentIdentifier, 'session_trace.harvestTimeSeconds') || 10
this.maxNodesPerHarvest = getConfigurationValue(agentIdentifier, 'session_trace.maxNodesPerHarvest') || 1000
/**
Expand Down Expand Up @@ -100,7 +99,7 @@ export class Aggregate extends AggregateBase {

if (prevMode === MODE.ERROR && this.#scheduler) {
this.trimSTNs(ERROR_MODE_SECONDS_WINDOW) // up until now, Trace would've been just buffering nodes up to max, which needs to be trimmed to last X seconds
this.#scheduler.runHarvest({})
this.#scheduler.runHarvest({ needResponse: true })
} else {
controlTraceOp(MODE.FULL)
}
Expand All @@ -120,7 +119,7 @@ export class Aggregate extends AggregateBase {
const stopTracePerm = () => {
if (sessionEntity.state.sessionTraceMode !== MODE.OFF) sessionEntity.write({ sessionTraceMode: MODE.OFF })
operationalGate.permanentlyDecide(false)
if (mostRecentModeKnown === MODE.FULL) this.#scheduler?.runHarvest({}) // allow queued nodes (past opGate) to final harvest, unless they were buffered in other modes
if (mostRecentModeKnown === MODE.FULL) this.#scheduler?.runHarvest() // allow queued nodes (past opGate) to final harvest, unless they were buffered in other modes
this.#scheduler?.stopTimer(true) // the 'true' arg here will forcibly block any future call to runHarvest, so the last runHarvest above must be prior
this.#scheduler = null
}
Expand All @@ -139,7 +138,7 @@ export class Aggregate extends AggregateBase {
this.ee.on(SESSION_EVENTS.RESUME, () => {
const updatedTraceMode = sessionEntity.state.sessionTraceMode
if (updatedTraceMode === MODE.OFF) stopTracePerm()
else if (updatedTraceMode === MODE.FULL && this.#scheduler && !this.#scheduler.started) this.#scheduler.runHarvest({})
else if (updatedTraceMode === MODE.FULL && this.#scheduler && !this.#scheduler.started) this.#scheduler.runHarvest({ needResponse: true })
mostRecentModeKnown = updatedTraceMode
})
this.ee.on(SESSION_EVENTS.PAUSE, () => { mostRecentModeKnown = sessionEntity.state.sessionTraceMode })
Expand Down Expand Up @@ -190,12 +189,13 @@ export class Aggregate extends AggregateBase {
retryDelay: this.harvestTimeSeconds
}, this)
this.#scheduler.harvest.on('resources', this.#prepareHarvest.bind(this))
if (dontStartHarvestYet === false) this.#scheduler.runHarvest({}) // sends first stn harvest immediately
if (dontStartHarvestYet === false) this.#scheduler.runHarvest({ needResponse: true }) // sends first stn harvest immediately
startupBuffer.decide(true) // signal to ALLOW & process data in EE's buffer into internal nodes queued for next harvest
}

#onHarvestFinished (result) {
if (result.sent && !result.failed && !this.#scheduler.started) { // continue interval harvest only after first call
if (result.sent && result.responseText && !this.ptid) { // continue interval harvest only if ptid was returned by server on the first
this.agentRuntime.ptid = this.ptid = result.responseText
this.#scheduler.startTimer(this.harvestTimeSeconds)
}

Expand All @@ -212,18 +212,15 @@ export class Aggregate extends AggregateBase {

#prepareHarvest (options) {
if (this.isStandalone) {
if (this.#scheduler.started) {
if (now() >= MAX_TRACE_DURATION) {
// Perform a final harvest once we hit or exceed the max session trace time
options.isFinalHarvest = true
this.operationalGate.permanentlyDecide(false)
this.#scheduler.stopTimer(true)
} else if (this.nodeCount <= REQ_THRESHOLD_TO_SEND && !options.isFinalHarvest) {
// Only harvest when more than some threshold of nodes are pending, after the very first harvest, with the exception of the last outgoing harvest.
return
}
if (this.ptid && now() >= MAX_TRACE_DURATION) {
// Perform a final harvest once we hit or exceed the max session trace time
options.isFinalHarvest = true
this.operationalGate.permanentlyDecide(false)
this.#scheduler.stopTimer(true)
} else if (this.ptid && this.nodeCount <= REQ_THRESHOLD_TO_SEND && !options.isFinalHarvest) {
// Only harvest when more than some threshold of nodes are pending, after the very first harvest, with the exception of the last outgoing harvest.
return
}
// else, we must be on the very first harvest (standalone mode), so go to next square
} else {
// -- *cli May '26 - Update: Not rate limiting backgrounded pages either for now.
// if (this.ptid && document.visibilityState === 'hidden' && this.nodeCount <= REQ_THRESHOLD_TO_SEND) return
Expand All @@ -234,6 +231,7 @@ export class Aggregate extends AggregateBase {
if (currentMode === MODE.OFF && Object.keys(this.trace).length === 0) return
if (currentMode === MODE.ERROR) return // Trace in this mode should never be harvesting, even on unload
}

return this.takeSTNs(options.retry)
}

Expand Down
1 change: 0 additions & 1 deletion src/loaders/configure/configure.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ export function configure (agent, opts = {}, loaderType, forceDrain) {
...(updatedInit.ajax.deny_list || []),
...(updatedInit.ajax.block_internal ? internalTrafficList : [])
]
runtime.ptid = agent.agentIdentifier
setRuntime(agent.agentIdentifier, runtime)

if (agent.api === undefined) agent.api = setAPI(agent.agentIdentifier, forceDrain, agent.runSoftNavOverSpa)
Expand Down
7 changes: 3 additions & 4 deletions tests/functional/stn/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,12 @@ testDriver.test('posts session traces', function (t, browser, router) {
let rumPromise = router.expectRum()
let resourcePromise = router.expectResources()
let loadPromise = browser.get(router.assetURL('lotsatimers.html')).waitForFeature('loaded')
let ptid

Promise.all([resourcePromise, rumPromise, loadPromise]).then(([{ request: { query } }]) => {
t.ok(+query.st > 1408126770885, `Got start time ${query.st}`)
ptid = query.ptid
t.ok(query.ptid, 'ptid on first harvest')
t.notok(query.ptid, 'No ptid on first harvest')
return router.expectResources().then(({ request: { query } }) => {
t.equals(query.ptid, ptid, `ptid on second harvest ${query.ptid}`)
t.ok(query.ptid, `ptid on second harvest ${query.ptid}`)
t.end()
})
}).catch(fail)
Expand Down
11 changes: 8 additions & 3 deletions tests/specs/harvesting/index.e2e.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { faker } from '@faker-js/faker'
import { testResourcesRequest } from '../../../tools/testing-server/utils/expect-tests'

describe('harvesting', () => {
it('should include the base query parameters', async () => {
Expand Down Expand Up @@ -45,15 +46,19 @@ describe('harvesting', () => {
})

it('should include the ptid query parameter on requests after the first session trace harvest', async () => {
const [rumRequest] = await Promise.all([
const ptid = faker.string.uuid()
browser.testHandle.scheduleReply('bamServer', {
test: testResourcesRequest,
body: ptid
})

await Promise.all([
browser.testHandle.expectRum(),
browser.testHandle.expectResources(),
browser.url(await browser.testHandle.assetURL('obfuscate-pii.html'))
.then(() => browser.waitForAgentLoad())
])

const ptid = rumRequest.request.query.ptid

const [
resourcesResults,
timingsResults,
Expand Down
25 changes: 23 additions & 2 deletions tests/specs/stn/retry-harvesting.e2e.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { faker } from '@faker-js/faker'
import { testResourcesRequest } from '../../../tools/testing-server/utils/expect-tests'

describe('stn retry harvesting', () => {
Expand All @@ -20,7 +21,7 @@ describe('stn retry harvesting', () => {
await browser.pause(500)
await browser.testHandle.clearScheduledReplies('bamServer')

const ptid = firstResourcesHarvest.request.query.ptid
const ptid = faker.string.uuid()
await browser.testHandle.scheduleReply('bamServer', {
test: testResourcesRequest,
permanent: true,
Expand All @@ -35,7 +36,7 @@ describe('stn retry harvesting', () => {

expect(firstResourcesHarvest.reply.statusCode).toEqual(statusCode)
expect(secondResourcesHarvest.request.body.res).toEqual(expect.arrayContaining(firstResourcesHarvest.request.body.res))
expect(secondResourcesHarvest.request.query.ptid).toEqual(ptid)
expect(secondResourcesHarvest.request.query.ptid).toBeUndefined()
expect(thirdResourcesHarvest.request.query.ptid).toEqual(ptid)
})
);
Expand All @@ -55,7 +56,27 @@ describe('stn retry harvesting', () => {
.then(() => browser.waitForAgentLoad())
])

// Pause a bit for browsers built-in automated retry logic crap
// await browser.pause(500)
// await browser.testHandle.clearScheduledReplies('bamServer')

// const ptid = faker.string.uuid()
// await browser.testHandle.scheduleReply('bamServer', {
// test: testResourcesRequest,
// permanent: true,
// body: ptid
// })

// const secondResourcesHarvest = await browser.testHandle.expectResources()
// const [thirdResourcesHarvest] = await Promise.all([
// browser.testHandle.expectResources(),
// $('#trigger').click()
// ])

expect(firstResourcesHarvest.reply.statusCode).toEqual(statusCode)
// expect(secondResourcesHarvest.request.body.res).not.toEqual(expect.arrayContaining(firstResourcesHarvest.request.body.res))
// expect(secondResourcesHarvest.request.query.ptid).toBeUndefined()
// expect(thirdResourcesHarvest.request.query.ptid).toEqual(ptid)
await expect(browser.testHandle.expectResources(10000, true)).resolves.toBeUndefined()
})
)
Expand Down
8 changes: 4 additions & 4 deletions tests/specs/stn/with-session-replay.e2e.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ describe.withBrowsersMatching(notIE)('stn with session replay', () => {
let initSTReceived = await loadPageAndGetResource(['stn/instrumented.html', { init: { privacy: { cookies_enabled: true }, session_replay: { enabled: false } } }], 3001)
let firstPageAgentVals = await getTraceValues()
expect(initSTReceived).toBeTruthy() // that is, trace should still fully run when the replay feature isn't around
expect(initSTReceived.request.query.ptid).not.toBeUndefined() // trace has ptid on first initial harvest
expect(initSTReceived.request.query.ptid).toBeUndefined() // trace doesn't have ptid on first initial harvest
expect(firstPageAgentVals).toEqual([true, MODE.FULL, expect.any(String)])

await navigateToRootDir()
Expand All @@ -94,7 +94,7 @@ describe.withBrowsersMatching(notIE)('stn with session replay', () => {
let secondPageAgentVals = await getTraceValues()
// On subsequent page load or refresh, trace should maintain the set mode, standalone, and same sessionid but have a new ptid corresponding to new page visit.
expect(secondInitST.request.query.s).toEqual(initSTReceived.request.query.s)
expect(secondInitST.request.query.ptid).not.toBeUndefined()
expect(secondInitST.request.query.ptid).toBeUndefined()
expect(secondPageAgentVals).toEqual([true, MODE.FULL, expect.any(String)]) // note it's expected & assumed that the replay mode is OFF

expect(secondPageAgentVals[2]).not.toEqual(firstPageAgentVals[2]) // ptids
Expand All @@ -118,7 +118,7 @@ describe.withBrowsersMatching(notIE)('stn with session replay', () => {
let initSTReceived = await loadPageAndGetResource(['stn/instrumented.html', config({ session_replay: replayConfig })], 3003)
let firstPageAgentVals = await getRuntimeValues()
expect(initSTReceived).toBeTruthy()
expect(initSTReceived.request.query.ptid).not.toBeUndefined()
expect(initSTReceived.request.query.ptid).toBeUndefined()
if (replayMode === 'OFF') {
expect(firstPageAgentVals).toEqual([true, MODE.FULL, true])
expect(Number(initSTReceived.request.query.hr)).toEqual(0)
Expand All @@ -134,7 +134,7 @@ describe.withBrowsersMatching(notIE)('stn with session replay', () => {
let secondPageAgentVals = await getRuntimeValues()
// On subsequent page load or refresh, trace should maintain FULL mode and session id.
expect(secondInitST.request.query.s).toEqual(initSTReceived.request.query.s)
expect(secondInitST.request.query.ptid).not.toBeUndefined() // this validates we're actually getting the 2nd page's initial res, not 1st page's unload res
expect(secondInitST.request.query.ptid).toBeUndefined() // this validates we're actually getting the 2nd page's initial res, not 1st page's unload res
if (replayMode === 'OFF') {
expect(secondPageAgentVals).toEqual([true, MODE.FULL, null]) // session_replay.featAggregate will be null as it's OFF and not imported on subsequent pages
expect(Number(secondInitST.request.query.hr)).toEqual(0)
Expand Down
7 changes: 1 addition & 6 deletions tests/unit/common/harvest/harvest.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ describe('_send', () => {
expect(xhrAddEventListener).toHaveBeenCalledWith('loadend', expect.any(Function), expect.any(Object))
expect(result).toEqual(jest.mocked(submitDataModule.xhr).mock.results[0].value)
expect(submitMethod).not.toHaveBeenCalled()
expect(spec.cbFinished).toHaveBeenCalledWith({ ...xhrState, sent: true, xhr: xhrState, failed: false, fullUrl: expect.any(String) })
expect(spec.cbFinished).toHaveBeenCalledWith({ ...xhrState, sent: true, xhr: xhrState, fullUrl: expect.any(String) })
})

test('should set cbFinished state retry to true with delay when xhr has 429 status', () => {
Expand All @@ -403,7 +403,6 @@ describe('_send', () => {
retry: true,
delay: harvestInstance.tooManyRequestsDelay,
xhr: xhrState,
failed: true,
fullUrl: expect.any(String)
})
})
Expand Down Expand Up @@ -431,7 +430,6 @@ describe('_send', () => {
sent: true,
retry: true,
xhr: xhrState,
failed: true,
fullUrl: expect.any(String)
})
})
Expand All @@ -458,7 +456,6 @@ describe('_send', () => {
...xhrState,
sent: true,
xhr: xhrState,
failed: false,
fullUrl: expect.any(String)
})
})
Expand Down Expand Up @@ -486,7 +483,6 @@ describe('_send', () => {
responseText: undefined,
sent: true,
xhr: xhrState,
failed: false,
fullUrl: expect.any(String)
})
})
Expand All @@ -511,7 +507,6 @@ describe('_send', () => {
...xhrState,
sent: false,
xhr: xhrState,
failed: true,
fullUrl: expect.any(String)
})
})
Expand Down

0 comments on commit 34b317f

Please sign in to comment.