Improve OpenTelemetry instrumentation to support adding REST client headers to span for REST call

[svifred]

I would like to add attributes, specifically client request and response headers, to the OpenTelemetry span for each REST call. For the OpenTelemetry Java agent instrumentation, I would take advantage of the configuration parameters

otel.instrumentation.http.client.capture-request-headers
otel.instrumentation.http.client.capture-response-headers

but the Quarkus instrumentation does not provide an implementation of these.

Per @brunobat and the Q&A discussion https://github.com/quarkusio/quarkus/discussions/44256, it would be possible to add support for these config parameters:

Th implementation should be straight forward. We need to create the new properties to control the feature and add attributes to the span in this class:

https://github.com/quarkusio/quarkus/blob/6ccff484bc930f393a97f61de3391a8b1967ece6/extensions/opentelemetry/runtime/src/main/java/io/quarkus/opentelemetry/runtime/tracing/intrumentation/restclient/OpenTelemetryClientFilter.java#L44

and this one as well:

https://github.com/quarkusio/quarkus/blob/e559cbdf93faac811d028f5af6acd5ad44b2f412/extensions/opentelemetry/runtime/src/main/java/io/quarkus/opentelemetry/runtime/tracing/intrumentation/resteasy/OpenTelemetryClassicServerFilter.java#L21

[quarkus-bot[bot]]

/cc @brunobat (opentelemetry), @cescoffier (rest-client), @geoand (rest-client), @radcortez (opentelemetry)

[Saphri]

We would love to have http headers and also the entity send/received

Headers raise a security concern as they often carry tokens ect. Entity logging can also be a problem if its not possible to control filtering and truncate size ect So make default not logging this as a start to keep it simple

Also it seems the otel things have changed with quarkus-rest based on vertx.. HttpInstrumenterVertxTracer seems to be the code responsible there

[geoand]

@brunobat this does not sounds like a good first issue to me - those are usually reserved to things that are very easily implemented and for which no (or very few) details remain to be worked out

[radcortez]

As a reminder, headers are Opt-In, meaning that they are not required to be supported by the implementation: https://opentelemetry.io/docs/specs/semconv/http/http-spans/

Also note:

_[11]: Instrumentations SHOULD require an explicit configuration of which headers are to be captured. Including all request headers can be a security risk - explicit configuration helps avoid leaking sensitive information. The User-Agent header is already captured in the useragent.original attribute. Users MAY explicitly configure instrumentations to capture them even though it is not recommended. The attribute value MUST consist of either multiple header values as an array of strings or a single-item array containing a possibly comma-concatenated string, depending on the way the HTTP library provides access to headers.

[geoand]

FWIW, we should not be putting any effort for new features into the quarkus-resteasy-client, all new features should go to the quarkus-rest-client

[geoand]

Also just to reiterate the point about good first issue, I am highly sceptical if anyone without intimite knowledge of the codebase can handle this :)

[geoand]

@brunobat @radcortez if we want this, I will take it as it's pretty involved.

[brunobat]

That's ok @geoand. This is just, get the headers and put them in the span attributes, along with a new property to switch it on/off. It's not trivial but easy enough. If quarkus-resteasy-client is not deprecated, we should have feature parity. If you want the issue, it's yours.

[geoand]

It's not trivial but easy enough.

That is not true unfortunately, because what we are after is controlling this option per-client - it doesn't make much sense to force the same setting for every client

[brunobat]

As a reminder, headers are Opt-In, meaning that they are not required to be supported by the implementation: https://opentelemetry.io/docs/specs/semconv/http/http-spans/

Also note:

_[11]: Instrumentations SHOULD require an explicit configuration of which headers are to be captured. Including all request headers can be a security risk - explicit configuration helps avoid leaking sensitive information. The User-Agent header is already captured in the useragent.original attribute. Users MAY explicitly configure instrumentations to capture them even though it is not recommended. The attribute value MUST consist of either multiple header values as an array of strings or a single-item array containing a possibly comma-concatenated string, depending on the way the HTTP library provides access to headers.

yes, but there's demand, I guess we can support it.

[brunobat]

It's not trivial but easy enough.

That is not true unfortunately, because what we are after is controlling this option per-client - it doesn't make much sense to force the same setting for every client

It's just 2x the work, not the complexity. And I don't see what we gain by not using the same property for all clients.

[geoand]

The REST Client is used to talk to disparate systems that have very different non-functional requirements. I never want our client experience to force global choices on users, for the simple reason that the alternative of not using the REST Client just (because one little thing isn't supported or breaks the workflow) is a vastly worse developer experience.

It's just 2x the work, not the complexity.

Not true, because the way things are implemented now, there is way to associate a traced Vertx HttpClient request with an instance of a REST Client.

[radcortez]

It's just 2x the work, not the complexity.

Not true, because the way things are implemented now, there is way to associate a traced Vertx HttpClient request with an instance of a REST Client.

Yes, it's annoying that it requires duplicate work, but for RESTEasy Classic, if I remember correctly, the implementation uses a plain filter, which already has access to all the request / response data, so it should be too hard to implement.

Another annoying part is that we don't differentiate between the Reactive and Classic configurations; it may be confusing for users to go through the docs and filter which one works with which.

On another note, we still need the Classic implementations to pass the TCKs and certify.