-
Notifications
You must be signed in to change notification settings - Fork 840
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
Return status code exceptions via CompletableResultCode in GrpcExporter and HttpExporter #6645
Return status code exceptions via CompletableResultCode in GrpcExporter and HttpExporter #6645
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. |
exporters/common/src/main/java/io/opentelemetry/exporter/internal/ExporterStatusException.java
Outdated
Show resolved
Hide resolved
d169788
to
0e19a77
Compare
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Thanks @jack-berg, I've made those changes. |
exporters/common/src/main/java/io/opentelemetry/exporter/internal/http/HttpExporter.java
Outdated
Show resolved
Hide resolved
b5557f4
to
039ca4c
Compare
Building off of @jack-berg's PR #6348, this PR uses the newly added
CompletableResultCode
'sfailExceptionally
method to return status code errors from theGrpcExporter
andHttpExporter
.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
GrpcExporter
andHttpExporter
s, 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 theinternal
package currently with the associated code.Please take a look - happy to make changes as necesssary.