-
Notifications
You must be signed in to change notification settings - Fork 917
Refactor http, grpc senders and promote to public API #7782
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
base: main
Are you sure you want to change the base?
Conversation
|
@jack-berg, I didn't forgot this issue. I'm planning to give you a response in the week of Nov. 17. |
brunobat
left a comment
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.
@jack-berg, thanks very much for this.
The end user changes are fair enough and after updating the Quarkus code, it worked out of the box and the IT tests with a real OTel Collector pass.
I'm adding a couple of comments.
|
|
||
| /** The gRPC response message bytes. */ | ||
| @SuppressWarnings("mutable") | ||
| byte[] getResponseMessage(); |
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.
I'm not sure what to do here because our gRPC API is reactive and I only have access to a Future. I wouldn't like to block here.
What can be done with this response?
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.
I also think the response is empty and that will return null during normal usage. Should we expect an Optional?
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.
Originally OTLP exporter responses were empty, but they now contain some optional information. See the proto definition for details.
Right now, the java implementation ignores anything in the response. I was trying to get ahead of future requirements to do something useful with the response by forcing sender implementations to provide a byte representation of the response body.
Thinking about it more, I think its possible to add this later, or to add a default implementation that just returns an empty byte array. The underlying exporter implementation could then try to do something useful if the response body is available, and skip if not.
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.
Sounds good to me.
| private final String type; | ||
| private final HttpSender httpSender; | ||
| private final ExporterInstrumentation exporterMetrics; | ||
| private final boolean exportAsJson; |
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.
I wonder if the JSON flag could be part of the HttpRequestBodyWriter
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.
Its embedded in there. The HttpRequestBodyWriter passed to HttpSender#send serializes the body in the appropriately based on the whether json is selected or not. All a sender has to do is ensure the correct Content-Type header is set by adhering to HttpSenderConfig#getContentType().
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.
I was just mentioning that because we could probably get rid of this parameter.
| * Additional headers that must be appended to every request. The resulting {@link Supplier} must | ||
| * be invoked for each request. | ||
| */ | ||
| Supplier<Map<String, List<String>>> getHeadersSupplier(); |
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.
note to me: handles auth
brunobat
left a comment
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.
I've taken a look at the declarative config being proposed. We currently don't use it but I have a few comments.
| * #getTrustManager()} will also be non-null. | ||
| */ | ||
| @Nullable | ||
| SSLContext getSslContext(); |
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.
This should probably be a configuration on the sender side because each sender will have it's own security wiring. In Quarkus we don't use these properties because the client is integrated in the framework and has it's own configs.
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.
This will be non-null if a user explicitly calls the various setters which change the security context, e.g.:
I understand that quarkus has its own security context integrated into the framework, but its a confusing experience for a user to try to explicitly set the security context and have those options ignored conditionally based on the sender.
Perhaps the quarkus sender can default to security context integrated into the framework, and only use this SSLContext when non-null.
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.
That's fair, as long as these properties are not used outside the sender. This could cause a split config problem.
I don't know how to ensure that.
| * #getSslContext()} will also be non-null. | ||
| */ | ||
| @Nullable | ||
| X509TrustManager getTrustManager(); |
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.
Same as above.
| * Additional headers that must be appended to every request. The resulting {@link Supplier} must | ||
| * be invoked for each request. | ||
| */ | ||
| Supplier<Map<String, List<String>>> getHeadersSupplier(); |
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.
I'm not sure a supplier should live in config objects, but probably that is a larger discussion related with declarative config.
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.
See comment here: #7782 (comment)
| * #getTrustManager()} will also be non-null. | ||
| */ | ||
| @Nullable | ||
| SSLContext getSslContext(); |
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.
Same as gRPC
| * #getSslContext()} will also be non-null. | ||
| */ | ||
| @Nullable | ||
| X509TrustManager getTrustManager(); |
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.
Same as gRPC
| * grpc-encoding}. | ||
| */ | ||
| @Nullable | ||
| Compressor getCompressor(); |
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.
At some point I'll need to create Compressor objects but the existing implementations are in internal packages.
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.
This PR promotes them to the stable package. The idea is that this config option gives you a ref to the resolved compressor, or null if none is configured. Hopefully the quarkus sender can integrate with the compressor, adding the appropriate headers and using it to compress the payload.
Why is this relavant for Quarkus? I'm asking because the spring starter doesn't care - and I'm trying to understand what the difference is |
There's actually nothing in here related to declarative config. The getters in As the set of configuration options we expose via the I would say that if a sender ignores any of those options, its technically not compliant. Non-compliant doesn't mean its not useful, but it does mean that the sender isn't able to behave like the user expects based on how they configured the builder. |
Because Quarkus has it's own senders based on a Vert.x client. It does not use OkHttp. See #6718 and its comments for more details. |
Thanks for the explanation @jack-berg. |
|
Ok so it seems like something close to this will work for quarkus. There are still some small tweaks (example), but its mostly correct. The next step will be to coordinate with @jkwatson and @open-telemetry/java-approvers to choose a strategy and schedule to get this merged. I opened this as one big PR to make the whole picture clear, but I'm happy to break it up in smaller more reviewable pieces as long as we can commit to work quickly to get all those pieces merged for a single release. This will be important to minimize churn. |
Resolves #6718.
Sketching out all the changes I think would be required to promote the HttpSender, GrpcSender interfaces to the public API. There's a lot. Summary:
Marshalerand related APIs, since they balloon the API surface area and would take lot of work to get ready for public. Introducing narrow focusedGrpcMessageWriter,HttpRequestBodyWriterto serve the function currently performed byMarshaler.MarshalerServiceStuband related APIs from the gRPC senders stuff. It drags in a bunch of unnecessary cruft andio.grpc.stubdependency.HttpSenderConfig#getExportAsJson()option. Senders don't need to be burdened with understanding whether the request is binary or JSON. They just need to be told the content type and a way to write the request body.Compressorand related APIs need to get promoted as well.GrpcSenderConfig,HttpSenderConfigObject GrpcSenderConfig#getManagedChannel(), which is required for backwards compatibility and forUpstreamGrpcSender, behind an internal-onlyExtendedGrpcSenderConfigLeaving as a draft because: