Skip to content

fix(inst-fetch,inst-xhr) Ignore network events with zero-timing #5332

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jan 29, 2025
Merged

fix(inst-fetch,inst-xhr) Ignore network events with zero-timing #5332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e19c6ee
fix(inst-fetch,inst-xhr) Ignore network events with zero-timing
chancancode Jan 10, 2025
fc0c81f
Merge branch 'main' into fix-5314
chancancode Jan 28, 2025
c99bb00
Merge branch 'main' into fix-5314
chancancode Jan 29, 2025
f7ab953
Merge branch 'main' into fix-5314
chancancode Jan 29, 2025
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
1 change: 1 addition & 0 deletions experimental/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ All notable changes to experimental packages in this project will be documented

* fix(instrumentation-grpc): monitor error events with events.errorMonitor [#5369](https://github.com/open-telemetry/opentelemetry-js/pull/5369) @cjihrig
* fix(exporter-metrics-otlp-http): browser OTLPMetricExporter was not passing config to OTLPMetricExporterBase super class [#5331](https://github.com/open-telemetry/opentelemetry-js/pull/5331) @trentm
* fix(instrumentation-fetch, instrumentation-xhr): Ignore network events with zero-timings [#5332](https://github.com/open-telemetry/opentelemetry-js/pull/5332) @chancancode
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where I put it for now; not sure if it should be put under breaking change instead, I suppose if you are doing something about these on the backend you may consider it breaking


### :books: (Refine Doc)

Expand Down
48 changes: 27 additions & 21 deletions experimental/packages/opentelemetry-instrumentation-fetch/test/fetch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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;
Copy link
Contributor Author

@chancancode chancancode Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is intended to "tweak" the timing but ends up affecting the body size, hence the changes to some of the expectations below (0/30/60). It's not great but #5130 and #5282 is actively working on unwinding these

}
});
Expand All @@ -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 {
Expand Down Expand Up @@ -458,7 +464,7 @@ describe('fetch', () => {
] as number;
assert.strictEqual(
responseContentLength,
30,
60,
`attributes ${SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH} is <= 0`
);

Expand Down Expand Up @@ -1217,7 +1223,7 @@ describe('fetch', () => {
] as number;
assert.strictEqual(
responseContentLength,
30,
60,
`attributes ${SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH} is <= 0`
);
});
Expand Down
53 changes: 31 additions & 22 deletions experimental/packages/opentelemetry-instrumentation-xml-http-request/test/xhr.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is intended to "tweak" the timing but ends up affecting the body size, hence the changes to some of the expectations below. It's not great but #5130 and #5282 is actively working on unwinding these

}
});
Expand Down Expand Up @@ -424,7 +433,7 @@ describe('xhr', () => {
] as number;
assert.strictEqual(
responseContentLength,
30,
60,
`attributes ${SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH} <= 0`
);
assert.strictEqual(
Expand Down Expand Up @@ -868,7 +877,7 @@ describe('xhr', () => {
] as number;
assert.strictEqual(
responseContentLength,
30,
60,
`attributes ${SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH} is <= 0`
);
});
Expand Down Expand Up @@ -1052,7 +1061,7 @@ describe('xhr', () => {
] as number;
assert.strictEqual(
responseContentLength,
0,
30,
`attributes ${SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH} <= 0`
);
assert.strictEqual(
Expand Down Expand Up @@ -1622,7 +1631,7 @@ describe('xhr', () => {
] as number;
assert.strictEqual(
responseContentLength,
30,
60,
`attributes ${SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH} <= 0`
);
assert.strictEqual(
Expand Down Expand Up @@ -2236,7 +2245,7 @@ describe('xhr', () => {
] as number;
assert.strictEqual(
responseContentLength,
0,
30,
`attributes ${SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH} <= 0`
);
assert.strictEqual(
Expand Down
1 change: 1 addition & 0 deletions packages/opentelemetry-sdk-trace-web/src/enums/PerformanceTimingNames.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
}
1 change: 1 addition & 0 deletions packages/opentelemetry-sdk-trace-web/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Expand Down
60 changes: 29 additions & 31 deletions packages/opentelemetry-sdk-trace-web/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,32 +58,22 @@ export function hasKey<O extends object>(
* @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: number | undefined;
let refTime: number | 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;
}

Expand All @@ -92,32 +82,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) {
Expand Down
Loading