OTLP/GRPC exporter implementation - Missing Reries on Exporter errors#677
OTLP/GRPC exporter implementation - Missing Reries on Exporter errors#677sanketmehta28 wants to merge 38 commits into
Conversation
…entation - Missing retries on Exporter errors
|
Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one. |
|
@sanketmehta28 |
|
There are some psalm errors appearing in CI |
| $delay = $this->retryPolicy->getDelay($attempt++); | ||
| if ($attempt > 0) { | ||
| self::logDebug('Waiting for ' . $delay . ' seconds before retrying export'); | ||
| sleep($delay); |
There was a problem hiding this comment.
I don't think sleeping here is a good idea, because it will cause the application being instrumented to wait. A better way might be to keep track of spans to retry (eg in internal storage), and check for spans to retry as a side-effect of doing something else in the exporter. We could do a final check as part of exporter shutdown to see if any pending spans need to be exported
Codecov Report
@@ Coverage Diff @@
## main #677 +/- ##
============================================
- Coverage 79.52% 79.43% -0.10%
- Complexity 1405 1434 +29
============================================
Files 160 162 +2
Lines 3468 3564 +96
============================================
+ Hits 2758 2831 +73
- Misses 710 733 +23
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Hi, Can anyone help me resolve this code coverage error? |
It's complaining about a decrease in code coverage, specifically in |
|
Just an explanation for not including fiber as of now: Also I have tried the default retry mechanism of GRPC which also blocks the exporter until retry times out or successful. .net otel module is using this same behaviour(PR) so it will not make much difference if this generic retry mechanism blocks the exporter. |
|
I think this needs an example of how to use and configure a retry policy, so that users can see it in action and understand how to configure (and disable/turn off) |
|
@tidal @Nevay: as you guys can see, most of the builds are getting failed because of doLog() function in LogsMessagetrait.php because doLog is also being defined in LoggerExporter.php but from loggerexporter.php it is trying to refer to doLog() function in LogsMessageTrait.php which is not right. I believe it is a minor bug and we need to create an issue for the same. |
I understand now... By adding |
I will change in loggerExporter in this PR |
…uments to ExponentialRetryPolicy constructor. Resolving minor review comments
…panExporterTrait. Removed 2 invalid GRPC status from retrying
| { | ||
| public function shouldRetry( | ||
| int $attempt, | ||
| int $status |
There was a problem hiding this comment.
I don't think this parameter is useful, will this ever be used for something besides checking for SpanExporterInterface::STATUS_FAILED_RETRYABLE?
There was a problem hiding this comment.
For current state, it is only being used for checking if the status is STATUS_FAILED_RETRYABLE. but this can be checked against a list of retryable codes for diff retry policies and exporters.
|
|
||
| while ($shouldRetry) { | ||
| // find the delay time before retry (it is 0 for the first attempt) | ||
| $delay = $this->retryPolicy->getDelay($attempt); |
There was a problem hiding this comment.
Having the retry logic in the trait instead of the exporter complicates handling Retry-After header & co.
OTLP/HTTP Throttling
There was a problem hiding this comment.
I have to put the retry logic in trait only because it is applicable to all span exporters and export() is actually implemented in that only.
There was a problem hiding this comment.
Where do you suggest I should implement this?
There was a problem hiding this comment.
@Nevay : Can you please give me an example where I can implement this?
There was a problem hiding this comment.
I think we can ignore this; exporters that have to handle Retry-After should implement their own retry logic instead of using this trait.
There was a problem hiding this comment.
ok I will move the logic to exporter
| ) { | ||
| $this->checkArgument( | ||
| $backoff_multiplier > 1, | ||
| sprintf('Backoff multiplier must be greater than 0: "%s" value provided', $backoff_multiplier) |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue has been automatically closed because it has not had recent activity, but it can be reopened. Thank you for your contributions. |
5 participants
Is your feature request related to a problem? Please describe.
The OTLP exporter specification says that transient errors MUST be handled with a retry strategy, where the retriable gRPC error codes are defined in the OTLP spec.
Fixes: #328
Describe the solution you'd like
I think we should implement generic retry policy which can be utilized by all exporters
Additional context
In grpc, retry is supported out of the box using service config but in this PR I have tried to provide the generic solution which can be used by all the exporters irrespective of the transport method.