Core: Add support for OpenTelemetry in HTTPClient #14360
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 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.
| public Builder withOpenTelemetry(OpenTelemetry openTelemetry) { | ||
| this.telemetry = openTelemetry; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
how is the openTelemetry configuration provided, via ENV ?
There was a problem hiding this comment.
Just passing OpenTelemetry instance is sufficient in this repository.
In Trino, we have 2 config properties to enable tracing:
tracing.enabled=true
tracing.exporter.endpoint=http://observe.example.com:4317| private final boolean isRootClient; | ||
| private final ConcurrentMap<Class<?>, ObjectReader> objectReaderCache = Maps.newConcurrentMap(); | ||
|
|
||
| private HTTPClient( |
There was a problem hiding this comment.
how does the E2E flows works does the Trino works, does it creates its own HTTPClient and in the builder sets its won OpenTelemetry client ?
There was a problem hiding this comment.
Right, Trino provides RESTClient builder in TrinoIcebergRestCatalogFactory.
The new code will be like this:
config -> HTTPClient.builder(config)
.uri(config.get(CatalogProperties.URI))
.withHeaders(RESTUtil.configHeaders(config))
.withOpenTelemetry(openTelemetry)
.build(),(openTelemetry will be injected in the constructor)
2 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.