update OTLP exporter transient errors table #1642
Conversation
There was a problem hiding this comment.
In general, the spec is usually more up to date than the respective OTEP. OTEPs are used for the initial discussion but as details go into the spec, changes applied there are usually not reflected back in the OTEP. @tigrannajaryan PTAL if this deviation is intended or not.
It sounds reasonable to remove them, however, as I don't think we would be able to recover from missing or misconfigured credentials unless authentication was turned off or permissions for the configured credentials were changed during runtime of the exporter. I'm not sure if this is likely enough so that consistently retrying is worth it.
specification/protocol/exporter.md
Outdated
| | 7 | Permission Denied | | ||
| | 8 | Resource Exhausted | | ||
| | 10 | Aborted | | ||
| | 10 | Out of Range | | ||
| | 14 | Unavailable | | ||
| | 15 | Data Loss | | ||
| | 16 | Unauthenticated | |
There was a problem hiding this comment.
We'll need to have a discussion if this change is considered breaking as this is declared stable already.
In theory, one could rely on exports being retried in these cases although I think it's unlikely.
|
specification/protocol/exporter.md is incorrect. specification/protocol/otlp.md is correct. I think a mistake happened when #699 added specification/protocol/exporter.md file to the spec. The retryable codes are defined by OTEP 0065: open-telemetry/oteps#65. Permission Denied and Unauthenticated were pointed out by @yurishkuro that they should be non-retryable and I agreed and that's how OTEP 0065 was approved. I would consider this a typo in We should probably remove the list of codes from IMO, this is not a breaking change in any way, just a typo correction in the spec. |
I agree.
Yes, that would be cleaner. @paivagustavo could you adapt the PR accordingly? Thanks! |
|
Let's make sure we also have this change listed in CHANGELOG so that anyone who implemented it incorrectly can fix. I believe it should be considered an implementation bug, no a breaking behavior change. |
…ication into otlp_exporter_transient_errors
|
Thanks for the review @arminru and @tigrannajaryan. I've addressed the suggestions made. I wasn't sure about which section of the CHANGELOG should be updated, so I created an |
| | HTTP Status Code | Description | | ||
| | ---------------- | ----------- | | ||
| | 408 | Request Timeout | | ||
| | 5xx | Server Errors | |
There was a problem hiding this comment.
The referenced section in otlp.md#failures actually does not specify this.
Could you a statement there pointing out that 408 and 5xx are considered retryable or otherwise leave this table here?
There was a problem hiding this comment.
Right, I've added a statement that point to that info. But I think It would be better if we have a full table just like the gRPC.
There was a problem hiding this comment.
This would be ideal yes. The current state is enough for this bugfix PR, however.
## Changes The transient errors table in the `protocol/exporter.md` specify `Permission Denied` and `Unauthenticated` errors as retryable, but they're defined as _permanent_ error in the [otep-35](https://github.com/open-telemetry/oteps/blob/main/text/0035-opentelemetry-protocol.md#export-response) and in the `protocol/otlp.md` file. ### Related issues This was found while implementing [retries in the go sdk](open-telemetry/opentelemetry-go#1832 (comment)).
Changes
The transient errors table in the
protocol/exporter.mdspecifyPermission DeniedandUnauthenticatederrors as retryable, but they're defined as permanent error in the otep-35 and in theprotocol/otlp.mdfile.Related issues
This was found while implementing retries in the go sdk.