-
Notifications
You must be signed in to change notification settings - Fork 806
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
feat(exporters)!: use transport interface in node.js exporters #4743
feat(exporters)!: use transport interface in node.js exporters #4743
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4743 +/- ##
==========================================
+ Coverage 91.04% 93.53% +2.49%
==========================================
Files 89 248 +159
Lines 1954 5789 +3835
Branches 416 1161 +745
==========================================
+ Hits 1779 5415 +3636
- Misses 175 374 +199 |
experimental/packages/exporter-logs-otlp-http/test/node/OTLPLogExporter.test.ts
Outdated
Show resolved
Hide resolved
getEnv().OTEL_EXPORTER_OTLP_LOGS_HEADERS | ||
), | ||
...parseHeaders(config?.headers), | ||
...USER_AGENT, |
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.
I guess this may change when #4447 is solved. Could you please add a reference here?
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.
@david-luna, sorry I think I've lost the train of thought from #4447.
Would you like me to add a reference that precedence may change
- here (as a comment)
- in Trace & Logs exporters to honour configuration spec for
OTEL_EXPORTER_OTLP_HEADERS
#4447 as an info that we changed it - in the
README.md(edit: meant CHANGELOG.md, sorry) that this was change? (I think we should do that anyway) 🤔
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.
Added a note in the CHANGELOG.md: bb5ec8c
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 @pichlermarc !
Once the exporters are finalized I'll check other SDKs and ask in the spec SIG about #4447 and the possibility to override the UA via env vars.
while (result.status === 'retryable' && attempts > 0) { | ||
attempts--; | ||
const upperBound = Math.min(nextBackoff, MAX_BACKOFF); | ||
const backoff = Math.random() * upperBound; |
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.
IIUC this is an iterative way of implementing exponential backoff (with 1,5 as exponent). What I do not fully get is this multiplication with Math.rand()
🤔
With this randomness in place the waiting time between retries may not increase. We may even get small values consuming the reties very fast.
- Maybe we need also a
lowerBound
? - Or maybe its even simpler if just use
Math.min(nextBackoff, MAX_BACKOFF)
? The progression is [1000, 1500, 2250, 3375, 5000, 5000 ...]
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.
Ah yes you're right, that does not make any sense. 🤦 I think I was trying to implement a jitter but then failed to follow through on it and ended up with this thing. I think I fixed it in in f1577c0.
It takes the min of nextBackoff
and MAX_BACKOFF
and then adds some jitter [-0.2ms, +0.2ms] to it. Would appreciate you having another look 👀
…telemetry#4743) * feat(exporters)!: use transport interface in node.js exporters * feat(exporters): hide compression property * feat(otlp-exporter-base)!: remove header property * feat(otlp-exporter-base): add retrying transport * fix: lint * chore: add changelog entry * fix: use queueMicrotask over nextTick * chore: move changelog entry to unreleased * chore: note that user-agent cannot be overwritten by users anymore * fix: export missing ExportResponseRetryable * fix: retry jitter
Which problem is this PR solving?
Exporters are very hard to test as everything has to be integration-tested at the moment (see #4116).
This PR does the following for the Node.js exporters:
ExporterTransport
interface previuously only used by the grpc exporters (introduced in fix(exporter-*-otlp-grpc)!: lazy load gRPC #4432) to the exporter base packageheader
property to reduce the API surface of theOTLPExporterNodeBase
and its child classescompression
property to reduce the API surface of theOTLPExporterNodeBase
and its child-classesA follow-up will do the same for browser transports, eventually removing the need to have different exporter-bases and retry implementations for Node.js and the Browser. This follow-up will conclude the tasks defined in #4116 and enabling implementation of feature-requests like #4631
Another follow-up will address config, splitting the config defaults and env-var code from the actual exporters. Then the tests will be adapted so that changes to underlying structures don't have to be propagated to the signal-/transport-specific exporters.
Breaking changes:
headers
was intended for internal use has been removed from all exporterscompression
was intended for internal use and has been removed from all exportershostname
was intended for use in tests and is not used by any exporters, it will be removed in a futuresingalSpecificHeaders
to be passed to the constructor.Updates #4116
Type of change
How Has This Been Tested?