Core: Add support for OpenTelemetry in HTTPClient#14360
Core: Add support for OpenTelemetry in HTTPClient#14360ebyhr wants to merge 3 commits intoapache:mainfrom
Conversation
| implementation libs.jackson.core | ||
| implementation libs.jackson.databind | ||
| implementation libs.caffeine | ||
| implementation libs.opentelemetry.httpclient |
There was a problem hiding this comment.
do we need to update and LICENCE ?
There was a problem hiding this comment.
I think we update the root LICENSE file only when we copy code from another library. Am I wrong?
There was a problem hiding this comment.
Not fully aware but i think if we ship a dependency : https://github.com/apache/iceberg/blob/main/aws-bundle/LICENSE we just include it i think, not an expert in this though, just thought to bring since we are introducing a new dependency in the project
There was a problem hiding this comment.
Ah, sorry I was only looking at LICENSE file in the root directory. Let me check other LICENSE files.
There was a problem hiding this comment.
@jbonofre Do you happen to know the answer to the question above?
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
| return this; | ||
| } | ||
|
|
||
| public Builder withOpenTelemetry(OpenTelemetry openTelemetry) { |
There was a problem hiding this comment.
do we need to protect this with a precondition of this being not null ?
|
|
||
| HttpClientBuilder clientBuilder = HttpClients.custom(); | ||
| HttpClientBuilder clientBuilder = | ||
| ApacheHttpClientTelemetry.builder(openTelemetry).build().newHttpClientBuilder(); |
There was a problem hiding this comment.
can we only set openTelemetry when there is something set via builder, i understand default value is NoOp but it would still add interceptor to call of HTTP but i guess it will do nothing, having set by builder would make sure its only added when required, WDYT ?
| ApacheHttpClientTelemetry.builder(openTelemetry).build().newHttpClientBuilder(); | |
| openTelemetry ? ApacheHttpClientTelemetry.builder(openTelemetry).build().newHttpClientBuilder() : HttpClients.custom(); |
gradle/libs.versions.toml
Outdated
| nessie = "0.106.0" | ||
| netty-buffer = "4.2.9.Final" | ||
| object-client-bundle = "3.3.2" | ||
| opentelemetry-httpclient = "2.20.1-alpha" |
There was a problem hiding this comment.
please check on the licence part, if we are ok and if we need to update it.
There was a problem hiding this comment.
Could you let me know how to check? https://github.com/apache/iceberg/blob/main/aws-bundle/LICENSE you mentioned before only contains dependencies used in aws-bundle module. iceberg-core module doesn't have LICENSE file.
There was a problem hiding this comment.
Ah i see, what i wanted to mostly know is.the following :
- can we take a dependency on this, based on some ASF doc search it seems like we very well can :)
- LICENCE : Apologies i am not an expert in this, just wanted to be cautious on this before we introduce a new dependency to project and update the LICENCE if required accordingly.
Lets do this : once the CI is green (i believe a70abe3 addresses my latest feedbacks :)) i will go ahead and approve ! (thanks a lot for change, i can truly see the benefits of this) and lets wait for sometime for some licence | dependency experts to weigh in (i will ping some folks on slack too) before we go ahead and merge, WDYT ?
There was a problem hiding this comment.
That works for me. Thanks for your help, as always :)
| nessie-client = { module = "org.projectnessie.nessie:nessie-client", version.ref = "nessie" } | ||
| netty-buffer = { module = "io.netty:netty-buffer", version.ref = "netty-buffer" } | ||
| object-client-bundle = { module = "com.emc.ecs:object-client-bundle", version.ref = "object-client-bundle" } | ||
| opentelemetry-httpclient = { module = "io.opentelemetry.instrumentation:opentelemetry-apache-httpclient-5.2", version.ref = "opentelemetry-httpclient" } |
There was a problem hiding this comment.
This pins an alpha OTel instrumentation artifact (2.20.1-alpha) and the module name is specifically opentelemetry-apache-httpclient-5.2 while Iceberg uses Apache HttpClient 5.6. Can we confirm this instrumentation is compatible with 5.6 (and that pulling opentelemetry-api-incubator alpha transitively is acceptable here)?
There was a problem hiding this comment.
Yes, it's compatible. OpenTelemetry repository tests against the base and the latest versions.
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
3 participants
I propose adding OpenTelemetry support to the HTTP client to improve traceability.
The screenshot shows the result of testing this PR with the Apache Polaris (incubating) and Trino.