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.

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

fix(instrumentation-fetch, instrumentation-xml-http-request) content length attributes now prese #5230

Merged
merged 13 commits into from
Dec 12, 2024
Merged

fix(instrumentation-fetch, instrumentation-xml-http-request) content length attributes now prese #5230

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
584febf
Make network events optional
arriIsHere Dec 3, 2024
9fbf814
utilize new property
arriIsHere Dec 3, 2024
228dc42
use new param for fetch
arriIsHere Dec 3, 2024
4c571bf
fix linting
arriIsHere Dec 3, 2024
f9ed04d
add test for fetch
arriIsHere Dec 3, 2024
e1d56d3
test for content length
arriIsHere Dec 3, 2024
bd5e6e3
test for utility function
arriIsHere Dec 3, 2024
e03f44e
Merge branch 'main' into main
arriIsHere Dec 4, 2024
12d20c3
Merge branch 'main' into main
arriIsHere Dec 5, 2024
d645e6c
update changelog
arriIsHere Dec 5, 2024
679a84e
remove wip code
arriIsHere Dec 6, 2024
6b1c370
Merge branch 'main' into main
arriIsHere Dec 6, 2024
43f5627
move to correct changelog
arriIsHere Dec 12, 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
16 changes: 10 additions & 6 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 @@ -112,9 +112,11 @@ export class FetchInstrumentation extends InstrumentationBase<FetchInstrumentati
},
api.trace.setSpan(api.context.active(), span)
);
if (!this.getConfig().ignoreNetworkEvents) {
web.addSpanNetworkEvents(childSpan, corsPreFlightRequest);
}
web.addSpanNetworkEvents(
childSpan,
corsPreFlightRequest,
this.getConfig().ignoreNetworkEvents
);
childSpan.end(
corsPreFlightRequest[web.PerformanceTimingNames.RESPONSE_END]
);
Expand Down Expand Up @@ -262,9 +264,11 @@ export class FetchInstrumentation extends InstrumentationBase<FetchInstrumentati
this._addChildSpan(span, corsPreFlightRequest);
this._markResourceAsUsed(corsPreFlightRequest);
}
if (!this.getConfig().ignoreNetworkEvents) {
web.addSpanNetworkEvents(span, mainRequest);
}
web.addSpanNetworkEvents(
span,
mainRequest,
this.getConfig().ignoreNetworkEvents
);
}
}

Expand Down
13 changes: 13 additions & 0 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 @@ -1212,5 +1212,18 @@ describe('fetch', () => {
const events = span.events;
assert.strictEqual(events.length, 0, 'number of events is wrong');
});

it('should still add the CONTENT_LENGTH attribute', () => {
const span: tracing.ReadableSpan = exportSpy.args[1][0][0];
const attributes = span.attributes;
const responseContentLength = attributes[
SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH
] as number;
assert.strictEqual(
responseContentLength,
30,
`attributes ${SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH} is <= 0`
);
});
});
});
16 changes: 10 additions & 6 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 @@ -149,9 +149,11 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase<XMLHttpRe
const childSpan = this.tracer.startSpan('CORS Preflight', {
startTime: corsPreFlightRequest[PTN.FETCH_START],
});
if (!this.getConfig().ignoreNetworkEvents) {
addSpanNetworkEvents(childSpan, corsPreFlightRequest);
}
addSpanNetworkEvents(
childSpan,
corsPreFlightRequest,
this.getConfig().ignoreNetworkEvents
);
childSpan.end(corsPreFlightRequest[PTN.RESPONSE_END]);
});
}
Expand Down Expand Up @@ -300,9 +302,11 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase<XMLHttpRe
this._addChildSpan(span, corsPreFlightRequest);
this._markResourceAsUsed(corsPreFlightRequest);
}
if (!this.getConfig().ignoreNetworkEvents) {
addSpanNetworkEvents(span, mainRequest);
}
addSpanNetworkEvents(
span,
mainRequest,
this.getConfig().ignoreNetworkEvents
);
}
}

Expand Down
13 changes: 13 additions & 0 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 @@ -857,6 +857,19 @@ describe('xhr', () => {
const events = span.events;
assert.strictEqual(events.length, 3, 'number of events is wrong');
});

it('should still add the CONTENT_LENGTH attribute', () => {
const span: tracing.ReadableSpan = exportSpy.args[1][0][0];
const attributes = span.attributes;
const responseContentLength = attributes[
SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH
] as number;
assert.strictEqual(
responseContentLength,
30,
`attributes ${SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH} is <= 0`
);
});
});
});

Expand Down
34 changes: 19 additions & 15 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 @@ -89,28 +89,32 @@ export function addSpanNetworkEvent(
}

/**
* Helper function for adding network events
* Helper function for adding network events and content length attributes
* @param span
* @param resource
* @param ignoreNetworkEvents
*/
export function addSpanNetworkEvents(
span: api.Span,
resource: PerformanceEntries
resource: PerformanceEntries,
ignoreNetworkEvents = false
Copy link
Member

@pichlermarc pichlermarc Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

hmm, that's really unfortunate.

I'd really like to avoid carrying this forward to 2.0 - @arriIsHere would you mind opening a follow-up PR towards the next branch later to implement option 1 "new helper method" from this comment there? It adds one more function but we'll be able to get rid of the body-size side-effect in this function, which I think cleans it up a bit.

I plans to move @opentelemetry/instrumentation-xhr and @opentelemetry/instrumentation-fetch into the same package as a follow up, which may even let us drop these functions from the public API of @opentelemetry/sdk-trace-web since they're not really SDK functions. That'd also go a long way of discouraging the anti-pattern of direct depending on the SDK for instrumentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely! I can finish that up this morning.

Copy link
Member

Choose a reason for hiding this comment

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

awesome, thank you 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pichlermarc opened #5257 against next

): void {
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);
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.CONNECT_END, resource);
addSpanNetworkEvent(span, PTN.REQUEST_START, resource);
addSpanNetworkEvent(span, PTN.RESPONSE_START, resource);
addSpanNetworkEvent(span, PTN.RESPONSE_END, resource);
const encodedLength = resource[PTN.ENCODED_BODY_SIZE];
if (encodedLength !== undefined) {
span.setAttribute(SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH, encodedLength);
Expand Down
29 changes: 29 additions & 0 deletions packages/opentelemetry-sdk-trace-web/test/utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,35 @@ describe('utils', () => {
} as PerformanceResourceTiming);
assert.strictEqual(addEventSpy.callCount, 9);
});

it('should ignore network events when ignoreNetworkEvents is true', () => {
const addEventSpy = sinon.spy();
const setAttributeSpy = sinon.spy();
const span = {
addEvent: addEventSpy,
setAttribute: setAttributeSpy,
} as unknown as tracing.Span;
const entries = {
[PTN.FETCH_START]: 123,
[PTN.DOMAIN_LOOKUP_START]: 123,
[PTN.DOMAIN_LOOKUP_END]: 123,
[PTN.CONNECT_START]: 123,
[PTN.SECURE_CONNECTION_START]: 123,
[PTN.CONNECT_END]: 123,
[PTN.REQUEST_START]: 123,
[PTN.RESPONSE_START]: 123,
[PTN.RESPONSE_END]: 123,
[PTN.DECODED_BODY_SIZE]: 123,
[PTN.ENCODED_BODY_SIZE]: 61,
} as PerformanceEntries;

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 only include encoded size when content encoding is being used', () => {
const addEventSpy = sinon.spy();
const setAttributeSpy = sinon.spy();
Expand Down
2 changes: 2 additions & 0 deletions semantic-conventions/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ All notable changes to the semantic-conventions package will be documented in th

### :bug: (Bug Fix)

* bug: content length attributes no longer get removed with `ignoreNetworkEvents: true` being set [#5229](https://github.com/open-telemetry/opentelemetry-js/issues/5229)

Copy link
Member

Choose a reason for hiding this comment

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

ah, that's the wrong changelog - it should go into CHANGELOG.md in the repository root 🙂

### :books: (Refine Doc)

### :house: (Internal)
Expand Down
Loading