feat(instrumentation-xml-http-request): support migration to stable HTTP semconv, v1.23.1#5662
Conversation
…TTP semconv, v1.23.1 Configure the instrumentation with semconvStabilityOptIn: 'http' to use the new, stable semconv v1.23.1 semantics or 'http/dup' for both old (v1.7.0) and stable semantics. When semconvStabilityOptIn is not specified (or does not contain these values), it uses the old semconv v1.7.0. I.e. the default behavior is unchanged. Closes: open-telemetry#5650 Refs: open-telemetry#5646
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5662 +/- ##
=======================================
Coverage 95.01% 95.01%
=======================================
Files 310 310
Lines 7971 7971
Branches 1609 1609
=======================================
Hits 7574 7574
Misses 397 397 🚀 New features to boost your workflow:
|
…nts in sdk-trace-web
| let knownMethods: { [key: string]: boolean }; | ||
| function getKnownMethods() { | ||
| if (knownMethods === undefined) { | ||
| const cfgMethods = getStringListFromEnv( |
There was a problem hiding this comment.
Is this usable in a web package? Or should this be configured programmatically like the OptIn config option?
There was a problem hiding this comment.
Good point. It'll "work" but get undefined everytime, because it'll get this implementation:
https://github.com/open-telemetry/opentelemetry-js/blob/v2.0.0/packages/opentelemetry-core/src/platform/browser/environment.ts#L29-L31
So this should probably change. BTW, the same code exists in instrumentation-fetch currently.
The spec says:
If the HTTP instrumentation could end up converting valid HTTP request methods to _OTHER, then it MUST provide a way to override the list of known HTTP methods. If this override is done via environment variable, then the environment variable MUST be named OTEL_INSTRUMENTATION_HTTP_KNOWN_METHODS and support a comma-separated list of case-sensitive known HTTP methods (this list MUST be a full override of the default known method, it is not a list of known methods in addition to the defaults).
I'm not sure if this instrumentation qualifies for If the HTTP instrumentation could end up converting valid HTTP request methods to _OTHER.
Thoughts?
The alternative would be to have a httpKnownMethods config option to this instrumentation.
There was a problem hiding this comment.
same code exists in instrumentation-fetch currently
Yeah I'm catching different things in each one, with lots to review some things don't stand out the first time around 😅 I guess a config option would be the way to do it, though I don't love that if we can help it. 🤔
There was a problem hiding this comment.
Since this is new and already in the fetch instr I think it's okay to let it be as-is and we can look into it for a follow-up, and fix both of them at the same time.
| instrumentations: [new XMLHttpRequestInstrumentation()], | ||
| instrumentations: [ | ||
| new XMLHttpRequestInstrumentation({ | ||
| semconvStabilityOptIn: 'http', |
There was a problem hiding this comment.
nit: is this optin config necessary in this test?
There was a problem hiding this comment.
No, not really.
It was necessary to make the old version of this test (see changes below) fail. Before this PR, this test was using the presence of the SEMATTRS_HTTP_RESPONSE_CONTENT_LENGTH span attr as a sign that PerformanceTiming resource usage was working (implementation in addSpanNetworkEvents from sdk-trace-web). When using only the newer semconv (via semconvStabilityOptIn: 'http',) that attr is not added -- for a different reason.
In a perfect "lots of tests have no maintenance or cognitive overhead"-world, this test would be run for all three OLD, STABLE, and DUPLICATE cases ... but I felt that was overkill given the exhaustive tests in xhr.test.ts.
…TTP semconv, v1.23.1 (open-telemetry#5662)
2 participants
Configure the instrumentation with semconvStabilityOptIn: 'http' to use the new, stable semconv v1.23.1 semantics or 'http/dup' for both old (v1.7.0) and stable semantics. When semconvStabilityOptIn is not specified (or does not contain these values), it uses the old semconv v1.7.0. I.e. the default behavior is unchanged.
Closes: #5650
Refs: #5646