fix(otlp-exporter-base): remove sendBeacon in favor of fetch with keepalive#6391
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6391 +/- ##
==========================================
+ Coverage 95.50% 95.59% +0.08%
==========================================
Files 365 361 -4
Lines 11617 11595 -22
Branches 2681 2683 +2
==========================================
- Hits 11095 11084 -11
+ Misses 522 511 -11
🚀 New features to boost your workflow:
|
9cf8b11 to
ed17d13
Compare
| */ | ||
|
|
||
| describe('OTLPTraceExporter', () => { | ||
| describe('OTLPMetricExporter', () => { |
There was a problem hiding this comment.
Typo seemed within scope.
| /** | ||
| * Track cumulative pending body size across all in-flight keepalive requests. | ||
| * This is necessary because the 64KiB limit is cumulative, not per-request. | ||
| */ | ||
| let pendingBodySize = 0; |
There was a problem hiding this comment.
This applies to all requests per renderer, so not just OTel requests from my understanding. I don't think there's a better solution here but am open to ideas.
There was a problem hiding this comment.
this looks good. once this is merged, we should also add an issue to remove the deprecated code eventually.
Would appreciate one additional review from @open-telemetry/browser-approvers before merging this :)
Edit: this probably supersedes #6358
| * (user-facing): environment variable configuration is no longer applied automatically when instantiating SDK components | ||
| (`LoggerProvider`, `BatchLogRecordProcessor`) directly from `@opentelemetry/sdk-logs`. Please migrate to using | ||
| `NodeSDK` from `@opentelemetry/sdk-node` to get automatic environment variable configuration. | ||
|
|
There was a problem hiding this comment.
deprecations are always weird to categorize. I usually just put a blip here to let people know what's coming. (feel free to ignore)
david-luna
left a comment
There was a problem hiding this comment.
looks good. A comment that you can skip if you want
experimental/packages/otlp-exporter-base/src/otlp-browser-http-export-delegate.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Marc Pichler <marc.pichler@dynatrace.com>
Which problem is this PR solving?
TLDR sendBeacon uses fetch+keepalive internally
navigator.sendBeacon()cannot see HTTP response codes, it returns whether the request was queued, not whether the server accepted it:This means server errors (503, 429) cause silent data loss with no opportunity to retry. The
RetryingTransportwrapper expects a'retryable'status to trigger retries, but sendBeacon could only return'success'or'failure', breaking the retry logic entirely.Additionally, when the browser's cumulative 64KB limit or 9 concurrent request limit is exceeded, sendBeacon rejects the request outright by returning
false. There's no way to fall back to a non-keepalive request that would still deliver the data.Fixes #3489
Fixes #6274
Short description of the changes
Replace
sendBeacontransport withfetch+keepaliveand track pending requests to stay within browser limits:createOtlpSendBeaconExportDelegate: Still works but delegates to fetch transportImpact
RetryingTransportMigration
For SDK users: No action required. Behavior improves automatically.
For custom transports: If using
createOtlpSendBeaconExportDelegate, it still works but is deprecated. Switch tocreateOtlpFetchExportDelegatewhen convenient.References
Type of change
How Has This Been Tested?
npm run compile && npm testpassesnpm run test:browserpassesChecklist: