[otlp] Grpc Status check and retry #6000
Conversation
…angaraj/opentelemetry-dotnet into rajrang/otlpAddOthExp
| { | ||
| var nextRetryDelayMilliseconds = retryDelayMilliseconds; | ||
|
|
||
| if (IsDeadlineExceeded(response.DeadlineUtc)) |
There was a problem hiding this comment.
Most of the code in this method is same as TryGetRetryResult
github-actions
bot
added
the
pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol
...xporter.OpenTelemetryProtocol/Implementation/Transmission/OtlpExporterTransmissionHandler.cs
Show resolved
Hide resolved
...Telemetry.Exporter.OpenTelemetryProtocol/Implementation/ExportClient/ExportClientResponse.cs
Show resolved
Hide resolved
...metry.Exporter.OpenTelemetryProtocol/Implementation/ExportClient/BaseOtlpGrpcExportClient.cs
Outdated
Show resolved
Hide resolved
...metry.Exporter.OpenTelemetryProtocol/Implementation/ExportClient/Grpc/GrpcProtocolHelpers.cs
Outdated
Show resolved
Hide resolved
| @@ -39,7 +39,7 @@ public override ExportClientResponse SendExportRequest(OtlpCollector.ExportMetri | |||
| { | |||
| OpenTelemetryProtocolExporterEventSource.Log.FailedToReachCollector(this.Endpoint, ex); | |||
|
|
|||
| return new ExportClientGrpcResponse(success: false, deadlineUtc: deadlineUtc, exception: ex); | |||
| return new ExportClientGrpcResponse(false, deadlineUtc, ex, null, null); | |||
There was a problem hiding this comment.
nit: Easier to read if you use named parameters.
return new ExportClientGrpcResponse(success: false, deadlineUtc: deadlineUtc, exception: ex, status: null, grpcStatusDetailsHeader: null);There was a problem hiding this comment.
This will be removed as follow up, hence kept simple.
| trailingHeaders = httpResponse.TrailingHeaders(); | ||
| status = GrpcProtocolHelpers.GetResponseStatus(httpResponse, trailingHeaders); |
There was a problem hiding this comment.
We did this work on line 66 do we need to do it again?
There was a problem hiding this comment.
The second call is necessary because trailing headers might not be fully available until the response stream is consumed. While we retrieve the status initially, gRPC often sends critical information like error details or final statuses in trailing headers, which can only be reliably accessed after reading the response body. This additional check ensures we handle cases where the gRPC status or errors are deferred until the stream is processed, improving robustness and accuracy. This follows the same logic as in Grpc library - https://github.com/grpc/grpc-dotnet/blob/5a58c24efc1d0b7c5ff88e7b0582ea891b90b17f/src/Grpc.Net.Client/Internal/GrpcCall.cs#L465
There was a problem hiding this comment.
👍 I pushed a commit to capture that info as a comment
| } | ||
|
|
||
| public Status? Status { get; } |
There was a problem hiding this comment.
Could we possibly switch this to also be non-nullable Status? I think it would improve the code readability to always have a status. Seems like there are only a couple spots where null gets passed but most paths define/pass something. We could introduce canned/static codes for the null ones? Not going to block for this, if you agree consider as maybe a follow-up/refactor 😄
Fixes Part of #5730
Design discussion issue #
Changes
Please provide a brief description of the changes here.
The current logic checks the HTTP response status for gRPC and makes retry decisions based on it, which is incorrect. We need to read the
grpc-statusheader to understand the status of the gRPC call. These changes are based on the following reference: https://github.com/grpc/grpc-dotnet/blob/5a58c24efc1d0b7c5ff88e7b0582ea891b90b17f/src/Grpc.Net.Client/Internal/GrpcCall.cs#L465Additionally, the
grpc-status-details-binheader value needs to be deserialized during retries to determine theGrpcRetryDelayfor the retry logic.Updated both in-memory and persistent storage tests to confirm there is no change in behavior.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes