-
Notifications
You must be signed in to change notification settings - Fork 858
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
Add HttpSender abstraction #5505
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #5505 +/- ##
============================================
+ Coverage 91.31% 91.34% +0.02%
- Complexity 4892 4893 +1
============================================
Files 547 546 -1
Lines 14412 14432 +20
Branches 1354 1351 -3
============================================
+ Hits 13161 13183 +22
+ Misses 866 863 -3
- Partials 385 386 +1
☔ View full report in Codecov by Sentry. |
exporters/common/src/main/java/io/opentelemetry/exporter/internal/http/HttpExporter.java
Outdated
Show resolved
Hide resolved
* @see HttpExporter | ||
* @see HttpExporterBuilder | ||
*/ | ||
public interface HttpSender { |
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 had to create this thing in multiple projects now. It's almost like the java ecosystem needs an http client equivalent of slf4j. 🤔
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.
jdk http client theoretically could put the "which http client" question to rest. Needs performance to be very competitive though, and probably full support for http2 (i.e. trailing headers)..
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.
And be based on an interface, not an abstract class. 😿
*/ | ||
void send( | ||
Consumer<OutputStream> marshaler, | ||
int contentLength, |
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.
since this is still internal, it's probably fine to just have this sitting here for now, but I would imagine we might want to create a "HttpMetadata" interface or something to pass in here, which might need to include more headers/etc in the future.
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.
Thanks!
Part of #5351.
This is the first of several phases. Adds HttpSender interface, with OkHttpHttpSender implementation. All of this still lives in
opentelemetry-exporter-common
so this is just a refactor with no functional change at this point.