feat(instrumentation): Add allowUrls config option to web instrumentation #4938
Conversation
jairo-mendoza
changed the title
jairo-mendoza
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4938 +/- ##
=======================================
Coverage 93.17% 93.18%
=======================================
Files 315 315
Lines 8086 8094 +8
Branches 1617 1619 +2
=======================================
+ Hits 7534 7542 +8
Misses 552 552
|
|
@pichlermarc Hi, all tests are now passing. Could I get another review and/or be placed in the merge queue? Thanks! |
| @@ -112,7 +112,7 @@ export { | |||
| export { merge } from './utils/merge'; | |||
| export { TracesSamplerValues } from './utils/sampling'; | |||
| export { TimeoutError, callWithTimeout } from './utils/timeout'; | |||
| export { isUrlIgnored, urlMatches } from './utils/url'; | |||
| export { isUrlAllowed, isUrlIgnored, urlMatches } from './utils/url'; | |||
There was a problem hiding this comment.
Hmm, I'd rather have this utility function duplicated across both @opentelemetry/instrumenation-fetch and @opentelemetry/instrumentation-xhr than it being part of the public API of a stable package. It gives us more flexibility to change it later if it's internal. Anything that is stable, we have to support 1 year after the next major so the timeline for changing/removing it would likely be > 1 year.
I'll ask the SIG what people think about combining @opentelemetry/instrumenation-fetch and @opentelemetry/instrumentation-xhr as that has recently come up with #4706 as well - if that goes through we may be able to quite easily de-dupe it when we merge the two packages. That's something for a different PR though.
There was a problem hiding this comment.
Gotcha, I'll make those changes soon. Thanks for the review!
Which problem is this PR solving?
This change provides easier control over which urls should have spans created for it. It can also be used with
ignoreUrlsfor more granular control.Fixes #4899
Short description of the changes
Added
allowUrlsas a config option to both theXMLHttpRequestInstrumentationandFetchInstrumentationclasses.Addition of the
isUrlAllowedfunction in the core package that both the fetch and xhr instrumentation will depend on.Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
opentelemetry-webexamples (fetchandxml-http-request) to accept multiple URLs. Then I added theallowUrlsconfig option on the instrumentation set up and verified that traces were being created (or not created).Checklist: