-
Notifications
You must be signed in to change notification settings - Fork 819
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.
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-xhr): http.url attribute should be absolute #3200
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3200 +/- ##
==========================================
- Coverage 93.29% 93.28% -0.02%
==========================================
Files 247 247
Lines 7352 7352
Branches 1512 1512
==========================================
- Hits 6859 6858 -1
- Misses 493 494 +1
|
Lack of changes in fetch instrumentation actually brings up a small but significant difference in the ignoreUrls config option of XHR and fetch instrumentations In fetch instrumentation already parsed URL is passed to _createSpan opentelemetry-js/experimental/packages/opentelemetry-instrumentation-fetch/src/fetch.ts Line 304 in a5abee6
In both instrumentations, ignoreUrls check is in _createSpan method, which means in fetch absolute url is always used while in xhr it depends on what URL was passed to XHR.open call. Therefore when using ignoreUrls you need to either have separate configs for these instrumentations (which sucks), or make sure your regexp works with both (unintuitive and undocumented) and have strings in both absolute and relative versions (string being exact match check is worse DX than having it just be substring/includes check</personal opinion>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this!
@@ -339,7 +339,7 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase<XMLHttpRe | |||
kind: api.SpanKind.CLIENT, | |||
attributes: { | |||
[SemanticAttributes.HTTP_METHOD]: method, | |||
[SemanticAttributes.HTTP_URL]: url, | |||
[SemanticAttributes.HTTP_URL]: parseUrl(url).toString(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking: we have several call sites of parseUrl
in the instrumentation. I believe we can parse the URL at the beginning of the span creation, and keep using that parsed URL to avoid duplicated parsing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The usages of pareUrl() looks like they are processing a past in property -- So I don't think they should be changed.
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
Which problem is this PR solving?
As per the specification
http.url
attribute should be "Full HTTP request URL in the formscheme://host[:port]/path?query[#fragment]
." Currently if a XHR request is done with a relative URL, then the http.url attribute is also relativeAlso added a test that checks this in fetch instrumentation
Short description of the changes
Added relative -> abs conversion via parseUrl
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Checklist: