-
Notifications
You must be signed in to change notification settings - Fork 834
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
Align GrpcSender contract with HttpSender #6658
Align GrpcSender contract with HttpSender #6658
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6658 +/- ##
============================================
- Coverage 90.10% 90.00% -0.10%
- Complexity 6287 6358 +71
============================================
Files 698 703 +5
Lines 18978 19145 +167
Branches 1861 1889 +28
============================================
+ Hits 17100 17232 +132
- Misses 1305 1334 +29
- Partials 573 579 +6 ☔ View full report in Codecov by Sentry. |
…y-java into fix-grpcsender-interface
void send(T request, Runnable onSuccess, BiConsumer<GrpcResponse, Throwable> onError); | ||
void send(T request, Consumer<GrpcResponse> onResponse, Consumer<Throwable> onError); |
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.
Yeah, this is a good catch.
exporters/common/src/main/java/io/opentelemetry/exporter/internal/grpc/GrpcExporter.java
Outdated
Show resolved
Hide resolved
exporters/common/src/main/java/io/opentelemetry/exporter/internal/grpc/GrpcExporter.java
Outdated
Show resolved
Hide resolved
exporters/common/src/main/java/io/opentelemetry/exporter/internal/grpc/GrpcExporter.java
Outdated
Show resolved
Hide resolved
...n/java/io/opentelemetry/exporter/sender/grpc/managedchannel/internal/UpstreamGrpcSender.java
Outdated
Show resolved
Hide resolved
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.
Looks good, I like the alignment. I had a few small suggestions, but nothing to block this. Thanks!
I was reviewing #6645 I was cringing at inconsistencies between GrpcSender and HttpSender.
void send(T request, Runnable onSuccess, BiConsumer<GrpcResponse, Throwable> onError)
void send(Marshaler marshaler, int contentLength, Consumer<Response> onResponse, Consumer<Throwable> onError);
HttpSender got it right by cleanly separating out the difference between an export request that received a response, and one that ended exceptionally. Its up to the caller HttpExporter to evaluate the response and decide whether it was a success or error response. For GrpcSender, these are muddied together in a
BiConsumer<GrpcResponse, Throwable>
.For #6645, when export attempts fail, we want to include exception in the
CompletableResultCode
which allow the caller to interpret what happened. Cleanly delineating between when a export failed but received a response, and failed exceptionally makes for a much cleaner API and user experience.