[exporter/otlp] Retry RESOURCE_EXHAUSTED only if the server returns RetryInfo

[svrakitin]

Description:

This makes us retry onResourceExhausted only if retry info is provided. It also makes code a bit more explicit.

In my case it caused an issue in production where upstream denied requests above max_recv_msg_size and we kept retrying.

Link to tracking Issue:
#1123

[codecov]

Codecov Report

Merging #5147 (ce65d16) into main (ef7754a) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5147      +/-   ##
==========================================
+ Coverage   90.52%   90.65%   +0.12%     
==========================================
  Files         187      187              
  Lines       11041    11052      +11     
==========================================
+ Hits         9995    10019      +24     
+ Misses        824      814      -10     
+ Partials      222      219       -3     

Impacted Files	Coverage Δ
exporter/otlpexporter/otlp.go	90.72% <100.00%> (+16.30%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef7754a...ce65d16. Read the comment docs.

[Aneurysm9]

Can we instead change that case to determine whether the retry info has been provided in the response? The spec requires retry of transient errors and says that RESOURCE_EXHAUSTED SHOULD be treated as transient. I think it makes sense to treat it as transient if retry info is provided and permanent otherwise.

[svrakitin]

@Aneurysm9 Makes sense, gonna work on this.

[svrakitin]

@Aneurysm9 I updated the PR to separate "always-retryable" and "never-retryable" error codes. Does it makes sense? Meanwhile I am going to write some tests for that as those seem to be missing (which turned out to be tricky).

[Aneurysm9]

I updated the PR to separate "always-retryable" and "never-retryable" error codes.

This seems like a good approach.

[bogdandrutu]

@tigrannajaryan

[tigrannajaryan]

I do not understand what this change does. A couple questions:

• Is the OTLP specification correct or something needs to be changed there?
• If the specification is correct which part of the specification is currently violated by the implementation in this repo that this PR fixes?

(Blocking temporarily to avoid accidental merging until all is clarified).

[svrakitin]

@tigrannajaryan This implementation effectively treats ResourceExhausted as transient (retryable) only if upstream signals backpressure. Otherwise, it will be treated as permanent.

It doesn't violate the spec as this only modifies the client behavior to drop data immediately if ResourceExhausted can't be retried. I would suggest to update the spec to recommend clients to retry on ResourceExhausted only if upstream signals backpressure.

[tigrannajaryan]

It doesn't violate the spec as this only modifies the client behavior to drop data immediately if ResourceExhausted can't be retried.

This does appear to contradict what the spec says:

The client SHOULD interpret gRPC status codes as retryable or not-retryable according to the following table:

|gRPC Code|Retryable?|
|---------|----------|
|RESOURCE_EXHAUSTED|Yes|

So, to make it clear, you are saying that RESOURCE_EXHAUSTED should not be an unconditional "Yes" in that table. It should be conditional:

• If the response includes a RetryInfo then RESOURCE_EXHAUSTED is retryable.
• If the response does not include a RetryInfo then RESOURCE_EXHAUSTED is not retryable.

Is this correct?

[svrakitin]

@tigrannajaryan Exactly.

[tigrannajaryan]

So, I think we need to update spec to reflect this. I believe this can be considered a bug in the specification, so it is an allowed change.

[svrakitin]

@tigrannajaryan Should I prepare the change to the spec before proceeding with this?

[tigrannajaryan]

@tigrannajaryan Should I prepare the change to the spec before proceeding with this?

Yes, that would be great.

[svrakitin]

I made the code follow the new addition to the spec and added the test to check RetryInfo/no-RetryInfo cases.

[tigrannajaryan]

Much better now, thanks.
Let's wait for open-telemetry/opentelemetry-specification#2480 to be merged before merging this.

Logic is clear now.

[tigrannajaryan]

@Aneurysm9 @bogdandrutu any further comments or good to merge?

[bogdandrutu]

@svrakitin please resolve conflicts, and I will merge

[bogdandrutu]

@svrakitin unit tests are failing

[svrakitin]

@bogdandrutu Yeah, I didn't notice that the tests now use go.uber.org/atomic. Updated the test.