Return status code exceptions via CompletableResultCode in GrpcExporter and HttpExporter#6645
Merged
jack-berg merged 5 commits intoopen-telemetry:mainfrom Aug 27, 2024
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6645 +/- ##
=========================================
Coverage 90.09% 90.09%
- Complexity 6280 6287 +7
=========================================
Files 697 698 +1
Lines 18951 18978 +27
Branches 1858 1861 +3
=========================================
+ Hits 17073 17099 +26
Misses 1305 1305
- Partials 573 574 +1 ☔ View full report in Codecov by Sentry. |
jack-berg
reviewed
Aug 19, 2024
exporters/common/src/main/java/io/opentelemetry/exporter/internal/ExporterStatusException.java
Outdated
Show resolved
Hide resolved
d169788 to
0e19a77
Compare
Contributor
Author
|
Thanks @jack-berg, I've updated my PR to include your suggestion. I also broke out the logging into a separate method. Take a look, I think I'm handling the gRPC error codes correctly (I looked at what |
jack-berg
reviewed
Aug 20, 2024
Member
jack-berg
left a comment
There was a problem hiding this comment.
Just a couple of small comments but this looks pretty good. Thanks!
exporters/common/src/main/java/io/opentelemetry/exporter/internal/FailedExportException.java
Outdated
Show resolved
Hide resolved
exporters/common/src/main/java/io/opentelemetry/exporter/internal/FailedExportException.java
Outdated
Show resolved
Hide resolved
exporters/common/src/main/java/io/opentelemetry/exporter/internal/grpc/GrpcExporter.java
Show resolved
Hide resolved
.../java/io/opentelemetry/exporter/otlp/testing/internal/AbstractGrpcTelemetryExporterTest.java
Outdated
Show resolved
Hide resolved
.../java/io/opentelemetry/exporter/otlp/testing/internal/AbstractHttpTelemetryExporterTest.java
Outdated
Show resolved
Hide resolved
Contributor
Author
|
Thanks @jack-berg, I've made those changes. |
jack-berg
reviewed
Aug 26, 2024
exporters/common/src/main/java/io/opentelemetry/exporter/internal/http/HttpExporter.java
Show resolved
Hide resolved
b5557f4 to
039ca4c
Compare
jack-berg
approved these changes
Aug 27, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Building off of @jack-berg's PR #6348, this PR uses the newly added
CompletableResultCode'sfailExceptionallymethod to return status code errors from theGrpcExporterandHttpExporter.The intention is to expose the status code upstream so it can be responded to, rather than simply have an 'unknown' failure. This was requested #6306. There was some discussion there around parsing response bodies which sounded like it was quite complicated; I have just focused on the basic case of returning the HTTP or GRPC status code.
This is my first PR into OpenTelemetry so please let me know if what I've done should be done differently or if you have suggestions for a better way.
I initially had separate exceptions for the
GrpcExporterandHttpExporters, but this seemed like overkill considering they did the same thing. Additionally, I wasn't sure if there was a better package for the exception itself - I've put it in theinternalpackage currently with the associated code.Please take a look - happy to make changes as necesssary.