Skip to content

fix(instrumentation-fetch): removes CORS sub-span creation as it was not compliant with ResourceTiming spec #5130

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

Closed
mariomc wants to merge 2 commits into from
Closed

fix(instrumentation-fetch): removes CORS sub-span creation as it was not compliant with ResourceTiming spec #5130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
598a57b
fix(instrumentation-fetch): removes CORS sub-span creation as it was …
mariomc Nov 7, 2024
19d233a
Merge branch 'main' into mg/fix-cors
mariomc Nov 20, 2024
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 @@ -129,6 +129,7 @@ All notable changes to experimental packages in this project will be documented

### :bug: (Bug Fix)

* fix(instrumentation-fetch): removes CORS sub-span creation as it was not compliant with ResourceTiming spec @mariomc
* fix(sampler-jaeger-remote): fixes an issue where package could emit unhandled promise rejections @Just-Sieb
* fix(otlp-grpc-exporter-base): default compression to `'none'` if env vars `OTEL_EXPORTER_OTLP_TRACES_COMPRESSION` and `OTEL_EXPORTER_OTLP_COMPRESSION` are falsy @sjvans
* fix(sdk-events): remove devDependencies to old `@opentelemetry/[email protected]`, `@opentelemetry/[email protected]` packages [#5013](https://github.com/open-telemetry/opentelemetry-js/pull/5013) @pichlermarc
Expand Down
29 changes: 0 additions & 29 deletions experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,30 +96,6 @@ export class FetchInstrumentation extends InstrumentationBase<FetchInstrumentati

init(): void {}

/**
* Add cors pre flight child span
* @param span
* @param corsPreFlightRequest
*/
private _addChildSpan(
span: api.Span,
corsPreFlightRequest: PerformanceResourceTiming
): void {
const childSpan = this.tracer.startSpan(
'CORS Preflight',
{
startTime: corsPreFlightRequest[web.PerformanceTimingNames.FETCH_START],
},
api.trace.setSpan(api.context.active(), span)
);
if (!this.getConfig().ignoreNetworkEvents) {
web.addSpanNetworkEvents(childSpan, corsPreFlightRequest);
}
childSpan.end(
corsPreFlightRequest[web.PerformanceTimingNames.RESPONSE_END]
);
}

/**
* Adds more attributes to span just before ending it
* @param span
Expand Down Expand Up @@ -257,11 +233,6 @@ export class FetchInstrumentation extends InstrumentationBase<FetchInstrumentati
const mainRequest = resource.mainRequest;
this._markResourceAsUsed(mainRequest);

const corsPreFlightRequest = resource.corsPreFlightRequest;
if (corsPreFlightRequest) {
this._addChildSpan(span, corsPreFlightRequest);
this._markResourceAsUsed(corsPreFlightRequest);
}
if (!this.getConfig().ignoreNetworkEvents) {
web.addSpanNetworkEvents(span, mainRequest);
}
Expand Down
74 changes: 13 additions & 61 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 @@ -139,7 +139,6 @@ function createFakePerformanceObs(url: string) {
const resources: PerformanceObserverEntryList = {
getEntries(): PerformanceEntryList {
return [
createResource({ name: absoluteUrl }) as any,
createMainResource({ name: absoluteUrl }) as any,
];
},
Expand Down Expand Up @@ -264,9 +263,6 @@ describe('fetch', () => {

const resources: PerformanceResourceTiming[] = [];
resources.push(
createResource({
name: fileUrl,
}),
createMainResource({
name: fileUrl,
})
Expand Down Expand Up @@ -389,7 +385,7 @@ describe('fetch', () => {
});

it('should create a span with correct root span', () => {
const span: tracing.ReadableSpan = exportSpy.args[1][0][0];
const span: tracing.ReadableSpan = exportSpy.args[0][0][0];
assert.strictEqual(
span.parentSpanId,
rootSpan.spanContext().spanId,
Expand All @@ -398,17 +394,17 @@ describe('fetch', () => {
});

it('span should have correct name', () => {
const span: tracing.ReadableSpan = exportSpy.args[1][0][0];
const span: tracing.ReadableSpan = exportSpy.args[0][0][0];
assert.strictEqual(span.name, 'HTTP GET', 'span has wrong name');
});

it('span should have correct kind', () => {
const span: tracing.ReadableSpan = exportSpy.args[1][0][0];
const span: tracing.ReadableSpan = exportSpy.args[0][0][0];
assert.strictEqual(span.kind, api.SpanKind.CLIENT, 'span has wrong kind');
});

it('span should have correct attributes', () => {
const span: tracing.ReadableSpan = exportSpy.args[1][0][0];
const span: tracing.ReadableSpan = exportSpy.args[0][0][0];
const attributes = span.attributes;
const keys = Object.keys(attributes);
assert.notStrictEqual(
Expand Down Expand Up @@ -473,50 +469,6 @@ describe('fetch', () => {
});

it('span should have correct events', () => {
const span: tracing.ReadableSpan = exportSpy.args[1][0][0];
const events = span.events;
assert.strictEqual(events.length, 8, 'number of events is wrong');
testForCorrectEvents(events, [
PTN.FETCH_START,
PTN.DOMAIN_LOOKUP_START,
PTN.DOMAIN_LOOKUP_END,
PTN.CONNECT_START,
PTN.CONNECT_END,
PTN.REQUEST_START,
PTN.RESPONSE_START,
PTN.RESPONSE_END,
]);
});

it('should create a span for preflight request', () => {
const span: tracing.ReadableSpan = exportSpy.args[0][0][0];
const parentSpan: tracing.ReadableSpan = exportSpy.args[1][0][0];
assert.strictEqual(
span.parentSpanId,
parentSpan.spanContext().spanId,
'parent span is not root span'
);
});

it('preflight request span should have correct name', () => {
const span: tracing.ReadableSpan = exportSpy.args[0][0][0];
assert.strictEqual(
span.name,
'CORS Preflight',
'preflight request span has wrong name'
);
});

it('preflight request span should have correct kind', () => {
const span: tracing.ReadableSpan = exportSpy.args[0][0][0];
assert.strictEqual(
span.kind,
api.SpanKind.INTERNAL,
'span has wrong kind'
);
});

it('preflight request span should have correct events', () => {
const span: tracing.ReadableSpan = exportSpy.args[0][0][0];
const events = span.events;
assert.strictEqual(events.length, 8, 'number of events is wrong');
Expand All @@ -533,7 +485,7 @@ describe('fetch', () => {
});

it('should set trace headers', () => {
const span: api.Span = exportSpy.args[1][0][0];
const span: api.Span = exportSpy.args[0][0][0];
assert.strictEqual(
lastResponse.headers[X_B3_TRACE_ID],
span.spanContext().traceId,
Expand Down Expand Up @@ -858,7 +810,7 @@ describe('fetch', () => {
});

it('span should have correct events', () => {
const span: tracing.ReadableSpan = exportSpy.args[1][0][0];
const span: tracing.ReadableSpan = exportSpy.args[0][0][0];
const events = span.events;
assert.strictEqual(events.length, 9, 'number of events is wrong');
testForCorrectEvents(events, [
Expand Down Expand Up @@ -913,7 +865,7 @@ describe('fetch', () => {
await prepare(url, span => {
span.setAttribute(CUSTOM_ATTRIBUTE_KEY, 'custom value');
});
const span: tracing.ReadableSpan = exportSpy.args[1][0][0];
const span: tracing.ReadableSpan = exportSpy.args[0][0][0];
const attributes = span.attributes;

assert.ok(attributes[CUSTOM_ATTRIBUTE_KEY] === 'custom value');
Expand All @@ -923,7 +875,7 @@ describe('fetch', () => {
await prepare(badUrl, span => {
span.setAttribute(CUSTOM_ATTRIBUTE_KEY, 'custom value');
});
const span: tracing.ReadableSpan = exportSpy.args[1][0][0];
const span: tracing.ReadableSpan = exportSpy.args[0][0][0];
const attributes = span.attributes;

assert.ok(attributes[CUSTOM_ATTRIBUTE_KEY] === 'custom value');
Expand Down Expand Up @@ -1022,7 +974,7 @@ describe('fetch', () => {
clearData();
});
it('should create a span with correct root span', () => {
const span: tracing.ReadableSpan = exportSpy.args[1][0][0];
const span: tracing.ReadableSpan = exportSpy.args[0][0][0];
assert.strictEqual(
span.parentSpanId,
rootSpan.spanContext().spanId,
Expand All @@ -1043,7 +995,7 @@ describe('fetch', () => {
});

it('should create a span with correct root span', () => {
const span: tracing.ReadableSpan = exportSpy.args[1][0][0];
const span: tracing.ReadableSpan = exportSpy.args[0][0][0];
assert.strictEqual(
span.parentSpanId,
rootSpan.spanContext().spanId,
Expand All @@ -1068,7 +1020,7 @@ describe('fetch', () => {

assert.strictEqual(
exportSpy.args.length,
2,
1,
`Wrong number of spans: ${exportSpy.args.length}`
);

Expand Down Expand Up @@ -1146,7 +1098,7 @@ describe('fetch', () => {

assert.strictEqual(
exportSpy.args.length,
2,
1,
`Wrong number of spans: ${exportSpy.args.length}`
);
assert.strictEqual(events.length, 8, 'number of events is wrong');
Expand Down Expand Up @@ -1208,7 +1160,7 @@ describe('fetch', () => {
clearData();
});
it('should NOT add network events', () => {
const span: tracing.ReadableSpan = exportSpy.args[1][0][0];
const span: tracing.ReadableSpan = exportSpy.args[0][0][0];
const events = span.events;
assert.strictEqual(events.length, 0, 'number of events is wrong');
});
Expand Down
27 changes: 0 additions & 27 deletions experimental/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ import {
import {
addSpanNetworkEvents,
getResource,
PerformanceTimingNames as PTN,
shouldPropagateTraceHeaders,
parseUrl,
} from '@opentelemetry/sdk-trace-web';
Expand Down Expand Up @@ -135,27 +134,6 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase<XMLHttpRe
});
}

/**
* Add cors pre flight child span
* @param span
* @param corsPreFlightRequest
* @private
*/
private _addChildSpan(
span: api.Span,
corsPreFlightRequest: PerformanceResourceTiming
): void {
api.context.with(api.trace.setSpan(api.context.active(), span), () => {
const childSpan = this.tracer.startSpan('CORS Preflight', {
startTime: corsPreFlightRequest[PTN.FETCH_START],
});
if (!this.getConfig().ignoreNetworkEvents) {
addSpanNetworkEvents(childSpan, corsPreFlightRequest);
}
childSpan.end(corsPreFlightRequest[PTN.RESPONSE_END]);
});
}

/**
* Add attributes when span is going to end
* @param span
Expand Down Expand Up @@ -295,11 +273,6 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase<XMLHttpRe
const mainRequest = resource.mainRequest;
this._markResourceAsUsed(mainRequest);

const corsPreFlightRequest = resource.corsPreFlightRequest;
if (corsPreFlightRequest) {
this._addChildSpan(span, corsPreFlightRequest);
this._markResourceAsUsed(corsPreFlightRequest);
}
if (!this.getConfig().ignoreNetworkEvents) {
addSpanNetworkEvents(span, mainRequest);
}
Expand Down
Loading