feat(otlp-exporter-http): add retries #2717
Conversation
Signed-off-by: Svetlana Brennan <[email protected]>
Signed-off-by: Svetlana Brennan <[email protected]>
svetlanabrennan
changed the title
Codecov Report
@@ Coverage Diff @@
## main #2717 +/- ##
==========================================
- Coverage 93.43% 92.51% -0.92%
==========================================
Files 159 75 -84
Lines 5452 2819 -2633
Branches 1145 541 -604
==========================================
- Hits 5094 2608 -2486
+ Misses 358 211 -147 |
Signed-off-by: Svetlana Brennan <[email protected]>
Signed-off-by: Svetlana Brennan <[email protected]>
| */ | ||
| export( | ||
| spans: ReadableSpan[], | ||
| resultCallback: (result: ExportResult) => void | ||
| resultCallback: (result: ExportResult) => void, | ||
| exportTimeoutMillis?: number, |
There was a problem hiding this comment.
We can't add optional parameter in the middle of the arguments because that would be a breaking change
There was a problem hiding this comment.
That makes sense. Thanks for your feedback.
| @@ -143,10 +143,6 @@ export abstract class BatchSpanProcessorBase<T extends BufferConfig> implements | |||
| return Promise.resolve(); | |||
| } | |||
| return new Promise((resolve, reject) => { | |||
| const timer = setTimeout(() => { | |||
There was a problem hiding this comment.
it looks like from the spec that both the BSP and the OTLP exporter have different timeout (30s for BSP and 10s pour otlp). I think we are meant to keep a global timeout at the BSP level and add another one in the OTLP exporter. WDYT ?
There was a problem hiding this comment.
Yes I agree. I just learned about that and solving issue #2706 first (adding and using the timeout in the OTLP exporter) is going to help me with this retry logic and I can avoid touching the BSP level timeout.
Signed-off-by: Svetlana Brennan <[email protected]>
Signed-off-by: Svetlana Brennan <[email protected]>
|
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. |
|
Not stale merely waiting on other things I think? |
|
Yes it's not stale. Waiting on timeout pr which will be ready for final review soon now that we're back to a single lerna monorepo. |
|
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. |
|
Not stale. Will update this pr soon now that timeout pr was approved/merged. |
|
Closing this draft pr in favor of this one: #3207 |
Which problem is this PR solving?
Trying to implement retry logic in the
exportmethod inside thetrace-otlp-httpexporter. I'm opening this draft PR to get some feedback on this solution. Any feedback would be greatly appreciated!The exporter spec is asking the retry logic to include an exponential back-off with jitter. Currently this solution doesn't include the "jitter" portion.
Changes made:
OTLPExporterBaseclass inexporter-trace-otlp-httpto include retry logicexportTimeoutMillisfrom theBatchSpanProcessorBaseclass in thesdk-trace-basetoOTLPExporterBaseclass inexporter-trace-otlp-httpSpanExporterinterface insdk-trace-baseto include optional parameters that are being used by theexportmethod in theOTLPExporterBaseclassFixes # 1233
Short description of the changes
Type of change
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
Tests not written yet. I tested this retry logic locally on an instrumented application.
Checklist: