Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[otlp] Add Retry Handler #5433

Merged

Conversation

vishweshbankwar
Copy link
Member

@vishweshbankwar vishweshbankwar commented Mar 9, 2024

Towards #1779
Design discussion issue #

Changes

Adds a retry transmission handler for retrying transient errors. It covers both http and grpc protocol.

Planning to do a follow up PR which will add the ability to opt-in for retries via an experimental environment variable.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

Copy link

codecov bot commented Mar 11, 2024

Codecov Report

Attention: Patch coverage is 78.26087% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 84.81%. Comparing base (6250307) to head (2d756ae).
Report is 127 commits behind head on main.

❗ Current head 2d756ae differs from pull request most recent head 572f529. Consider uploading reports for the commit 572f529 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5433      +/-   ##
==========================================
+ Coverage   83.38%   84.81%   +1.42%     
==========================================
  Files         297      284      -13     
  Lines       12531    12206     -325     
==========================================
- Hits        10449    10352      -97     
+ Misses       2082     1854     -228     
Flag Coverage Δ
unittests ?
unittests-Solution-Experimental 84.78% <78.26%> (?)
unittests-Solution-Stable 84.74% <78.26%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...rotocol/Implementation/Transmission/RetryHelper.cs 100.00% <100.00%> (ø)
...ansmission/OtlpExporterRetryTransmissionHandler.cs 80.00% <80.00%> (ø)
...yProtocol/Implementation/ExportClient/OtlpRetry.cs 88.57% <40.00%> (+5.81%) ⬆️

... and 60 files with indirect coverage changes

@vishweshbankwar vishweshbankwar marked this pull request as ready for review March 11, 2024 20:17
@vishweshbankwar vishweshbankwar requested a review from a team March 11, 2024 20:17
@vishweshbankwar vishweshbankwar added the pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package label Mar 11, 2024
…n/Transmission/OtlpExporterRetryTransmissionHandler.cs

Co-authored-by: Utkarsh Umesan Pillai <[email protected]>
@@ -186,16 +188,6 @@ public static IEnumerable<object[]> GetGrpcTestCases()
},
expectedRetryAttempts: 9),
};

yield return new[]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alanwest - I did not quite understand the significance of this test. It was testing that we would retry if the deadline is not set and server responds with a very high delay. We should not run in to this case as we would always have the deadline set, let me know if I am missing something here.

vishweshbankwar and others added 2 commits March 11, 2024 17:07
…llectorIntegrationTests.cs

Co-authored-by: Utkarsh Umesan Pillai <[email protected]>
@utpilla utpilla merged commit ff6725f into open-telemetry:main Mar 12, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants