-
Notifications
You must be signed in to change notification settings - Fork 912
feat(browser otlp exporter): add fetch transport for fetch-only environments #5807
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
base: main
Are you sure you want to change the base?
feat(browser otlp exporter): add fetch transport for fetch-only environments #5807
Conversation
…on, xhr and fetch delegates
2c4a8b6
to
e3cd53a
Compare
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.
Seems reasonable to me and I don't see any obvious issues. I'd wait for @pichlermarc review before merging because he's much closer to the exporters but overall LGTM
experimental/packages/otlp-exporter-base/src/transport/fetch-transport.ts
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5807 +/- ##
=======================================
Coverage 95.01% 95.01%
=======================================
Files 303 303
Lines 7946 7946
Branches 1607 1607
=======================================
Hits 7550 7550
Misses 396 396 🚀 New features to boost your workflow:
|
: 'cors' | ||
: 'no-cors', | ||
}); | ||
clearTimeout(timeout); |
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.
Looks like it's possible you won't clear the timeout if something in the try
block throws. Consider moving this to a finally
block?
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.
Thanks for your review, I made the change. Let me know if I can do anything more.
Hi guys, any news on this? |
Which problem is this PR solving?
In service workers,
XMLHttpRequest
andnavigator.sendBeacon
are both unavailable, making theOTLPTraceExporter
from@opentelemetry/exporter-trace-otlp-http
unusable.Fixes #4631
Short description of the changes
Add the fetch transport and enables it only when both xhr and beacons are unavailable.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
I took the xhr transport tests and adapted the
arrange
step for the fetch transport.See the test file: experimental/packages/otlp-exporter-base/test/browser/fetch-transport.test.ts
Checklist:
Additional context