From 3893d45987a96da5c8e8cc3c70349188fcac5c9c Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Fri, 10 Jan 2025 15:14:18 -0800 Subject: [PATCH] fix(inst-fetch,inst-xhr) Ignore network events with zero-timing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Background: 1. For historical reasons, the perf/resource timing spec uses 0 as a special value to denote when some timing information is either not applicable (e.g. no redirect occured) or not available (when producing an opaque resource timing object for CORS resources). 2. However, in some limited cases, 0 can also be a legitimate value for these timing events. Note that this is rare in real life – these fields are high-res performance timestamp relative to the performance time-origin, which is typically the navigation event for the initial page load. For a _resource_ timing to have a 0 timestamp, it would have to be initiated simultaneously with page load, it is unclear if this can actually happen IRL. Reportedly (#1769), at one point this was observed in some browsers during automated tests where things happen very fast and the browser artifically supress the timing resolution. It was unclear if the report was about the navigation timing entry or resource timing entries. It is also unclear if these utilities are intended for anything other than the internal fetch/XHR instrumentation, but they are public API, so if someone uses these functions on the initial page navigation event, then it is possible for the function to see legitimate 0-value inputs. 3. When creating span events, we do not use the timeOrigin-relative perf timestamps as-is. Rather, we convert them back to the UNIX epoch timestamps. When this conversion is applied to those 0 timestamps, it creates nonsensical/misleading events that are quite difficult to distinguish for downstream consumers. It would be nice if the W3C specs would have left the N/A values as `undefined`, but that's not the world we live in and so we have to work with what we've got. History: 1. Initially, the code ignored 0-value timestamps. 2. #1769 identified cases of valid 0-value timestamps and removed the check. 3. This causes the other category of bugs where we created the nonsensical events (#2457, #3848, #4478) 4. #3897 added a special-case for `secureConnectionStart` by way of tapping into auxiliary metadata 5. That appoach cannot be generalized for the other cases, so #4486 added some rather convoluted checks. 6. As part of refactoring the tests to use service workers, a new bug #5314 was identified. Presumably, the patch in #4486 was writen that way (as opposed to just checking for 0) to avoid breaking #1769, but I suspect it ends up breaking some of those use cases anyway. Options: 1. Keep the patch from #4486 largely as-is but change the reference time from `fetchStart` to `startTime`. 2. Revert to the very original intent of the code and drop everything with 0-values. 3. This commit attempts a compromise position, when `startTime === 0` (which probably means we are called with the initial navigation event), 0-values are plausibly valid so we include them by default, but in any other cases (`startTime > 0`), legitimate 0-values should not be possible so we drop them by default. Part of the issue here is that it's unclear how these utilities are being used since they are public APIs. In core, these functions are only called by the fetch/xhr instrumentation with resource timing entries, where legitimate 0-value seems extremely unlikely. In my opinion, the rarity of legitimate 0-value timing values in the real world (recall that it doesn't just mean something happened very fast, but something happened very fast _at the same instant that the page loaded_) should yield to the conflicting interst (to avoid non-sensical events that are difficult to process downstream) here, especially when it appears that the only observed cases are from automated testsing. Personally I would be in favor of the stronger position (2), but attempted to strike a balance here to keep things useful the other cases. In #2457, @johnbley argued: > I think that for this piece of tech, our telemetry should report > whatever the browser says with minimal processing, and let > downstream/backend/easier-to-modify components deal with > normalization/truncation/processing if desired (e.g., throwing > away absurd times like "that page took 6 years or -5 hours to > load", or declaring that "no redirects happened so the redirect > timing is not 0 but non-existent"). Does that not work in your > world for some reason? I could see this perspective if the "minimal processing" means that we sends the 0-values to the backends as 0s. It's still unfortunate but at least it would be easy to write a query/filter to process them away. However, given we are actively normalizes the 0-values into the absolute epoch timestamp of `performance.timeOrigin`, I think this becomes actively hostile to backend/downstream processing, as it turns a relatively clear signal (value=0) into something that would require stateful machinary to reverse engineer (infer the timeOrigin from a trace), or imperfect huristics (ignore things that seems to take way too long). It also takes up bytesize/bandwidth to transmit in the vast majority of cases where it would be actively unhelpful. Instead, I'd argue it creates way less harm to drop the 0-values. When a 0-value is expected but not sent, it may skew the distrubtion on some aggregrate metrics slightly (e.g. making "average time for SSL handshake" appears large), but probably nothing would break. If we are expecting backends to do work to normalize data anyway, then IMO it would also be way easier for them to notice the missing items and insert it back than the other way around. Fixes #3199 Fixes #5314 See also #1769, #2457, #3848, #3879, #4478, #4486 --- .../test/fetch.test.ts | 50 +++-- .../test/xhr.test.ts | 53 +++-- .../src/enums/PerformanceTimingNames.ts | 1 + .../opentelemetry-sdk-trace-web/src/types.ts | 1 + .../opentelemetry-sdk-trace-web/src/utils.ts | 60 +++--- .../test/utils.test.ts | 195 +++++++++++------- 6 files changed, 209 insertions(+), 151 deletions(-) diff --git a/experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts b/experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts index f81336ae4f6..5ea37a3586e 100644 --- a/experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts +++ b/experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts @@ -90,27 +90,27 @@ const textToReadableStream = (msg: string): ReadableStream => { const CUSTOM_ATTRIBUTE_KEY = 'span kind'; const defaultResource = { - connectEnd: 15, - connectStart: 13, - decodedBodySize: 0, - domainLookupEnd: 12, - domainLookupStart: 11, - encodedBodySize: 0, - fetchStart: 10.1, + entryType: 'resource', + name: '', initiatorType: 'fetch', - nextHopProtocol: '', - redirectEnd: 0, + startTime: 10.1, redirectStart: 0, + redirectEnd: 0, + workerStart: 0, + fetchStart: 10.1, + domainLookupStart: 11, + domainLookupEnd: 12, + connectStart: 13, + secureConnectionStart: 0, + connectEnd: 15, requestStart: 16, - responseEnd: 20.5, responseStart: 17, - secureConnectionStart: 14, + responseEnd: 20.5, + duration: 10.4, + decodedBodySize: 30, + encodedBodySize: 30, transferSize: 0, - workerStart: 0, - duration: 0, - entryType: '', - name: '', - startTime: 0, + nextHopProtocol: '', }; function createResource(resource = {}): PerformanceResourceTiming { @@ -124,7 +124,7 @@ function createResource(resource = {}): PerformanceResourceTiming { function createMainResource(resource = {}): PerformanceResourceTiming { const mainResource: any = createResource(resource); Object.keys(mainResource).forEach((key: string) => { - if (typeof mainResource[key] === 'number') { + if (typeof mainResource[key] === 'number' && mainResource[key] !== 0) { mainResource[key] = mainResource[key] + 30; } }); @@ -139,8 +139,14 @@ function createFakePerformanceObs(url: string) { const resources: PerformanceObserverEntryList = { getEntries(): PerformanceEntryList { return [ - createResource({ name: absoluteUrl }) as any, - createMainResource({ name: absoluteUrl }) as any, + createResource({ + name: absoluteUrl, + [PTN.SECURE_CONNECTION_START]: url.startsWith('https:') ? 14 : 0, + }) as any, + createMainResource({ + name: absoluteUrl, + [PTN.SECURE_CONNECTION_START]: url.startsWith('https:') ? 14 : 0, + }) as any, ]; }, getEntriesByName(): PerformanceEntryList { @@ -458,7 +464,7 @@ describe('fetch', () => { ] as number; assert.strictEqual( responseContentLength, - 30, + 60, `attributes ${SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH} is <= 0` ); @@ -468,6 +474,8 @@ describe('fetch', () => { it('span should have correct events', () => { const span: tracing.ReadableSpan = exportSpy.args[1][0][0]; const events = span.events; + console.log(events.length, JSON.stringify(events, null, 2)); + assert.strictEqual(events.length, 8, 'number of events is wrong'); testForCorrectEvents(events, [ PTN.FETCH_START, @@ -1217,7 +1225,7 @@ describe('fetch', () => { ] as number; assert.strictEqual( responseContentLength, - 30, + 60, `attributes ${SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH} is <= 0` ); }); diff --git a/experimental/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts b/experimental/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts index cd9d18e84b7..823d6f942e9 100644 --- a/experimental/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts +++ b/experimental/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts @@ -134,28 +134,37 @@ const postData = ( function createResource(resource = {}): PerformanceResourceTiming { const defaultResource = { - connectEnd: 15, - connectStart: 13, - decodedBodySize: 0, - domainLookupEnd: 12, - domainLookupStart: 11, - encodedBodySize: 0, - fetchStart: 10.1, + entryType: 'resource', + name: '', initiatorType: 'xmlhttprequest', - nextHopProtocol: '', - redirectEnd: 0, + startTime: 10.1, redirectStart: 0, + redirectEnd: 0, + workerStart: 0, + fetchStart: 10.1, + domainLookupStart: 11, + domainLookupEnd: 12, + connectStart: 13, + secureConnectionStart: 0, + connectEnd: 15, requestStart: 16, - responseEnd: 20.5, responseStart: 17, - secureConnectionStart: 14, + responseEnd: 20.5, + duration: 10.4, + decodedBodySize: 30, + encodedBodySize: 30, transferSize: 0, - workerStart: 0, - duration: 0, - entryType: '', - name: '', - startTime: 0, + nextHopProtocol: '', }; + + if ( + 'name' in resource && + typeof resource.name === 'string' && + resource.name.startsWith('https:') + ) { + defaultResource.secureConnectionStart = 14; + } + return Object.assign( {}, defaultResource, @@ -166,7 +175,7 @@ function createResource(resource = {}): PerformanceResourceTiming { function createMainResource(resource = {}): PerformanceResourceTiming { const mainResource: any = createResource(resource); Object.keys(mainResource).forEach((key: string) => { - if (typeof mainResource[key] === 'number') { + if (typeof mainResource[key] === 'number' && mainResource[key] !== 0) { mainResource[key] = mainResource[key] + 30; } }); @@ -424,7 +433,7 @@ describe('xhr', () => { ] as number; assert.strictEqual( responseContentLength, - 30, + 60, `attributes ${SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH} <= 0` ); assert.strictEqual( @@ -868,7 +877,7 @@ describe('xhr', () => { ] as number; assert.strictEqual( responseContentLength, - 30, + 60, `attributes ${SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH} is <= 0` ); }); @@ -1052,7 +1061,7 @@ describe('xhr', () => { ] as number; assert.strictEqual( responseContentLength, - 0, + 30, `attributes ${SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH} <= 0` ); assert.strictEqual( @@ -1622,7 +1631,7 @@ describe('xhr', () => { ] as number; assert.strictEqual( responseContentLength, - 30, + 60, `attributes ${SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH} <= 0` ); assert.strictEqual( @@ -2236,7 +2245,7 @@ describe('xhr', () => { ] as number; assert.strictEqual( responseContentLength, - 0, + 30, `attributes ${SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH} <= 0` ); assert.strictEqual( diff --git a/packages/opentelemetry-sdk-trace-web/src/enums/PerformanceTimingNames.ts b/packages/opentelemetry-sdk-trace-web/src/enums/PerformanceTimingNames.ts index 6730e5e2c11..252562af981 100644 --- a/packages/opentelemetry-sdk-trace-web/src/enums/PerformanceTimingNames.ts +++ b/packages/opentelemetry-sdk-trace-web/src/enums/PerformanceTimingNames.ts @@ -35,6 +35,7 @@ export enum PerformanceTimingNames { RESPONSE_END = 'responseEnd', RESPONSE_START = 'responseStart', SECURE_CONNECTION_START = 'secureConnectionStart', + START_TIME = 'startTime', UNLOAD_EVENT_END = 'unloadEventEnd', UNLOAD_EVENT_START = 'unloadEventStart', } diff --git a/packages/opentelemetry-sdk-trace-web/src/types.ts b/packages/opentelemetry-sdk-trace-web/src/types.ts index 076665cc3f6..00664234d08 100644 --- a/packages/opentelemetry-sdk-trace-web/src/types.ts +++ b/packages/opentelemetry-sdk-trace-web/src/types.ts @@ -35,6 +35,7 @@ export type PerformanceEntries = { [PerformanceTimingNames.RESPONSE_END]?: number; [PerformanceTimingNames.RESPONSE_START]?: number; [PerformanceTimingNames.SECURE_CONNECTION_START]?: number; + [PerformanceTimingNames.START_TIME]?: number; [PerformanceTimingNames.UNLOAD_EVENT_END]?: number; [PerformanceTimingNames.UNLOAD_EVENT_START]?: number; }; diff --git a/packages/opentelemetry-sdk-trace-web/src/utils.ts b/packages/opentelemetry-sdk-trace-web/src/utils.ts index e8fe4ef1d69..a1f7fcccc2f 100644 --- a/packages/opentelemetry-sdk-trace-web/src/utils.ts +++ b/packages/opentelemetry-sdk-trace-web/src/utils.ts @@ -59,32 +59,22 @@ export function hasKey( * @param span * @param performanceName name of performance entry for time start * @param entries - * @param refPerfName name of performance entry to use for reference + * @param ignoreZeros */ export function addSpanNetworkEvent( span: api.Span, performanceName: string, entries: PerformanceEntries, - refPerfName?: string + ignoreZeros = true ): api.Span | undefined { - let perfTime = undefined; - let refTime = undefined; if ( hasKey(entries, performanceName) && - typeof entries[performanceName] === 'number' + typeof entries[performanceName] === 'number' && + !(ignoreZeros && entries[performanceName] === 0) ) { - perfTime = entries[performanceName]; - } - const refName = refPerfName || PTN.FETCH_START; - // Use a reference time which is the earliest possible value so that the performance timings that are earlier should not be added - // using FETCH START time in case no reference is provided - if (hasKey(entries, refName) && typeof entries[refName] === 'number') { - refTime = entries[refName]; - } - if (perfTime !== undefined && refTime !== undefined && perfTime >= refTime) { - span.addEvent(performanceName, perfTime); - return span; + return span.addEvent(performanceName, entries[performanceName]); } + return undefined; } @@ -93,32 +83,40 @@ export function addSpanNetworkEvent( * @param span * @param resource * @param ignoreNetworkEvents + * @param ignoreZeros */ export function addSpanNetworkEvents( span: api.Span, resource: PerformanceEntries, - ignoreNetworkEvents = false + ignoreNetworkEvents = false, + ignoreZeros?: boolean ): void { + if (ignoreZeros === undefined) { + ignoreZeros = resource[PTN.START_TIME] !== 0; + } + if (!ignoreNetworkEvents) { - addSpanNetworkEvent(span, PTN.FETCH_START, resource); - addSpanNetworkEvent(span, PTN.DOMAIN_LOOKUP_START, resource); - addSpanNetworkEvent(span, PTN.DOMAIN_LOOKUP_END, resource); - addSpanNetworkEvent(span, PTN.CONNECT_START, resource); - if ( - hasKey(resource as PerformanceResourceTiming, 'name') && - (resource as PerformanceResourceTiming)['name'].startsWith('https:') - ) { - addSpanNetworkEvent(span, PTN.SECURE_CONNECTION_START, resource); - } - addSpanNetworkEvent(span, PTN.CONNECT_END, resource); - addSpanNetworkEvent(span, PTN.REQUEST_START, resource); - addSpanNetworkEvent(span, PTN.RESPONSE_START, resource); - addSpanNetworkEvent(span, PTN.RESPONSE_END, resource); + addSpanNetworkEvent(span, PTN.FETCH_START, resource, ignoreZeros); + addSpanNetworkEvent(span, PTN.DOMAIN_LOOKUP_START, resource, ignoreZeros); + addSpanNetworkEvent(span, PTN.DOMAIN_LOOKUP_END, resource, ignoreZeros); + addSpanNetworkEvent(span, PTN.CONNECT_START, resource, ignoreZeros); + addSpanNetworkEvent( + span, + PTN.SECURE_CONNECTION_START, + resource, + ignoreZeros + ); + addSpanNetworkEvent(span, PTN.CONNECT_END, resource, ignoreZeros); + addSpanNetworkEvent(span, PTN.REQUEST_START, resource, ignoreZeros); + addSpanNetworkEvent(span, PTN.RESPONSE_START, resource, ignoreZeros); + addSpanNetworkEvent(span, PTN.RESPONSE_END, resource, ignoreZeros); } + const encodedLength = resource[PTN.ENCODED_BODY_SIZE]; if (encodedLength !== undefined) { span.setAttribute(SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH, encodedLength); } + const decodedLength = resource[PTN.DECODED_BODY_SIZE]; // Spec: Not set if transport encoding not used (in which case encoded and decoded sizes match) if (decodedLength !== undefined && encodedLength !== decodedLength) { diff --git a/packages/opentelemetry-sdk-trace-web/test/utils.test.ts b/packages/opentelemetry-sdk-trace-web/test/utils.test.ts index 0011dde1485..46e9ed33c26 100644 --- a/packages/opentelemetry-sdk-trace-web/test/utils.test.ts +++ b/packages/opentelemetry-sdk-trace-web/test/utils.test.ts @@ -100,6 +100,7 @@ describe('utils', () => { setAttribute: setAttributeSpy, } as unknown as tracing.Span; const entries = { + [PTN.START_TIME]: 123, [PTN.FETCH_START]: 123, [PTN.DOMAIN_LOOKUP_START]: 123, [PTN.DOMAIN_LOOKUP_END]: 123, @@ -113,21 +114,13 @@ describe('utils', () => { [PTN.ENCODED_BODY_SIZE]: 61, } as PerformanceEntries; + assert.strictEqual(setAttributeSpy.callCount, 0); assert.strictEqual(addEventSpy.callCount, 0); addSpanNetworkEvents(span, entries); assert.strictEqual(setAttributeSpy.callCount, 2); - //secure connect start should not be added to non-https resource - assert.strictEqual(addEventSpy.callCount, 8); - //secure connect start should be added to an https resource - addEventSpy.resetHistory(); - addSpanNetworkEvents(span, { - ...entries, - name: 'https://foo', - } as PerformanceResourceTiming); assert.strictEqual(addEventSpy.callCount, 9); }); - it('should ignore network events when ignoreNetworkEvents is true', () => { const addEventSpy = sinon.spy(); const setAttributeSpy = sinon.spy(); @@ -136,6 +129,7 @@ describe('utils', () => { setAttribute: setAttributeSpy, } as unknown as tracing.Span; const entries = { + [PTN.START_TIME]: 123, [PTN.FETCH_START]: 123, [PTN.DOMAIN_LOOKUP_START]: 123, [PTN.DOMAIN_LOOKUP_END]: 123, @@ -149,13 +143,100 @@ describe('utils', () => { [PTN.ENCODED_BODY_SIZE]: 61, } as PerformanceEntries; + assert.strictEqual(setAttributeSpy.callCount, 0); assert.strictEqual(addEventSpy.callCount, 0); addSpanNetworkEvents(span, entries, true); assert.strictEqual(setAttributeSpy.callCount, 2); - //secure connect start should not be added to non-https resource assert.strictEqual(addEventSpy.callCount, 0); }); + it('should ignore zero timings by default', () => { + const addEventSpy = sinon.spy(); + const setAttributeSpy = sinon.spy(); + const span = { + addEvent: addEventSpy, + setAttribute: setAttributeSpy, + } as unknown as tracing.Span; + const entries = { + [PTN.START_TIME]: 123, + [PTN.FETCH_START]: 123, + [PTN.DOMAIN_LOOKUP_START]: 0, + [PTN.DOMAIN_LOOKUP_END]: 0, + [PTN.CONNECT_START]: 0, + [PTN.SECURE_CONNECTION_START]: 0, + [PTN.CONNECT_END]: 0, + [PTN.REQUEST_START]: 0, + [PTN.RESPONSE_START]: 0, + [PTN.RESPONSE_END]: 130, + [PTN.DECODED_BODY_SIZE]: 0, + [PTN.ENCODED_BODY_SIZE]: 0, + } as PerformanceEntries; + + assert.strictEqual(setAttributeSpy.callCount, 0); + assert.strictEqual(addEventSpy.callCount, 0); + + addSpanNetworkEvents(span, entries); + assert.strictEqual(setAttributeSpy.callCount, 1); + assert.strictEqual(addEventSpy.callCount, 2); + }); + it('should not ignore zero timings by default if startTime = 0', () => { + const addEventSpy = sinon.spy(); + const setAttributeSpy = sinon.spy(); + const span = { + addEvent: addEventSpy, + setAttribute: setAttributeSpy, + } as unknown as tracing.Span; + const entries = { + [PTN.START_TIME]: 0, + [PTN.FETCH_START]: 0, + [PTN.DOMAIN_LOOKUP_START]: 0, + [PTN.DOMAIN_LOOKUP_END]: 0, + [PTN.CONNECT_START]: 0, + [PTN.SECURE_CONNECTION_START]: 0, + [PTN.CONNECT_END]: 1, + [PTN.REQUEST_START]: 2, + [PTN.RESPONSE_START]: 3, + [PTN.RESPONSE_END]: 4, + [PTN.DECODED_BODY_SIZE]: 123, + [PTN.ENCODED_BODY_SIZE]: 61, + } as PerformanceEntries; + + assert.strictEqual(setAttributeSpy.callCount, 0); + assert.strictEqual(addEventSpy.callCount, 0); + + addSpanNetworkEvents(span, entries); + assert.strictEqual(setAttributeSpy.callCount, 2); + assert.strictEqual(addEventSpy.callCount, 9); + }); + it('should not ignore zero timings if ignoreZeros = false', () => { + const addEventSpy = sinon.spy(); + const setAttributeSpy = sinon.spy(); + const span = { + addEvent: addEventSpy, + setAttribute: setAttributeSpy, + } as unknown as tracing.Span; + const entries = { + [PTN.START_TIME]: 123, + [PTN.FETCH_START]: 123, + [PTN.DOMAIN_LOOKUP_START]: 0, + [PTN.DOMAIN_LOOKUP_END]: 0, + [PTN.CONNECT_START]: 0, + [PTN.SECURE_CONNECTION_START]: 0, + [PTN.CONNECT_END]: 0, + [PTN.REQUEST_START]: 0, + [PTN.RESPONSE_START]: 0, + [PTN.RESPONSE_END]: 130, + [PTN.DECODED_BODY_SIZE]: 0, + [PTN.ENCODED_BODY_SIZE]: 0, + } as PerformanceEntries; + + assert.strictEqual(setAttributeSpy.callCount, 0); + assert.strictEqual(addEventSpy.callCount, 0); + + addSpanNetworkEvents(span, entries, false, false); + assert.strictEqual(setAttributeSpy.callCount, 1); + assert.strictEqual(addEventSpy.callCount, 9); + }); it('should only include encoded size when content encoding is being used', () => { const addEventSpy = sinon.spy(); const setAttributeSpy = sinon.spy(); @@ -177,7 +258,7 @@ describe('utils', () => { }); }); describe('addSpanNetworkEvent', () => { - [0, -2, 123].forEach(value => { + [-2, 123].forEach(value => { describe(`when entry is ${value}`, () => { it('should add event to span', () => { const addEventSpy = sinon.spy(); @@ -200,122 +281,82 @@ describe('utils', () => { }); }); }); - describe('when entry is not numeric', () => { - it('should NOT add event to span', () => { + describe(`when entry is zero`, () => { + it('should not add event to span by default', () => { const addEventSpy = sinon.spy(); const span = { addEvent: addEventSpy, } as unknown as tracing.Span; const entries = { - [PTN.FETCH_START]: 'non-numeric', - } as unknown; + [PTN.SECURE_CONNECTION_START]: 0, + } as PerformanceEntries; assert.strictEqual(addEventSpy.callCount, 0); - addSpanNetworkEvent( - span, - PTN.FETCH_START, - entries as PerformanceEntries - ); + addSpanNetworkEvent(span, PTN.SECURE_CONNECTION_START, entries); assert.strictEqual(addEventSpy.callCount, 0); }); - }); - describe('when entries does NOT contain the performance', () => { - it('should NOT add event to span', () => { + it('should add event to span if ignoreZeros = false', () => { const addEventSpy = sinon.spy(); const span = { addEvent: addEventSpy, } as unknown as tracing.Span; const entries = { - [PTN.FETCH_START]: 123, + [PTN.SECURE_CONNECTION_START]: 0, } as PerformanceEntries; assert.strictEqual(addEventSpy.callCount, 0); - addSpanNetworkEvent(span, 'foo', entries); + addSpanNetworkEvent(span, PTN.SECURE_CONNECTION_START, entries, false); - assert.strictEqual( - addEventSpy.callCount, - 0, - 'should not call addEvent' - ); + assert.strictEqual(addEventSpy.callCount, 1); + const args = addEventSpy.args[0]; + + assert.strictEqual(args[0], 'secureConnectionStart'); + assert.strictEqual(args[1], 0); }); }); - describe('when entries contain invalid performance timing', () => { - it('should only add events with time greater that or equal to reference value to span', () => { + describe('when entry is not numeric', () => { + it('should NOT add event to span', () => { const addEventSpy = sinon.spy(); const span = { addEvent: addEventSpy, } as unknown as tracing.Span; const entries = { - [PTN.FETCH_START]: 123, // default reference time - [PTN.CONNECT_START]: 0, - [PTN.REQUEST_START]: 140, - } as PerformanceEntries; + [PTN.FETCH_START]: 'non-numeric', + } as unknown; assert.strictEqual(addEventSpy.callCount, 0); - addSpanNetworkEvent(span, PTN.CONNECT_START, entries); - - assert.strictEqual( - addEventSpy.callCount, - 0, - 'should not call addEvent' + addSpanNetworkEvent( + span, + PTN.FETCH_START, + entries as PerformanceEntries ); - addSpanNetworkEvent(span, PTN.REQUEST_START, entries); - - assert.strictEqual( - addEventSpy.callCount, - 1, - 'should call addEvent for valid value' - ); + assert.strictEqual(addEventSpy.callCount, 0); }); }); - - describe('when entries contain invalid performance timing and a reference event', () => { - it('should only add events with time greater that or equal to reference value to span', () => { + describe('when entries does NOT contain the performance', () => { + it('should NOT add event to span', () => { const addEventSpy = sinon.spy(); const span = { addEvent: addEventSpy, } as unknown as tracing.Span; const entries = { - [PTN.FETCH_START]: 120, - [PTN.CONNECT_START]: 120, // this is used as reference time here - [PTN.REQUEST_START]: 10, + [PTN.FETCH_START]: 123, } as PerformanceEntries; assert.strictEqual(addEventSpy.callCount, 0); - addSpanNetworkEvent( - span, - PTN.REQUEST_START, - entries, - PTN.CONNECT_START - ); + addSpanNetworkEvent(span, 'foo', entries); assert.strictEqual( addEventSpy.callCount, 0, 'should not call addEvent' ); - - addSpanNetworkEvent(span, PTN.FETCH_START, entries, PTN.CONNECT_START); - - assert.strictEqual( - addEventSpy.callCount, - 1, - 'should call addEvent for valid value' - ); - - addEventSpy.resetHistory(); - addSpanNetworkEvent(span, PTN.CONNECT_START, entries, 'foo'); // invalid reference , not adding event to span - assert.strictEqual( - addEventSpy.callCount, - 0, - 'should not call addEvent for invalid reference(non-existent)' - ); }); }); });