Use OpenTelemetry Instrumenter

[radcortez]

• Fixes Refactor OTel integration to use the Instrumenter API #1572

[radcortez]

This is not totally done yet, but I wanted to get some feedback before moving forward.

I've tried to only replace the current existing code with the OTel APIs. It seems to work for Kafka and AMQP, but the RabbitMQ one is lacking tests, so no way to verify them at the moment (I'll need to add them).

What I don't like is the Tracing code is all over the place. And each connector is handling it in different ways. I'm wondering if we should introduce a SPI (like Vert.x did for tracing stuff) and make this well defined (it can also be used for gathering Metrics).

On a separate topic, I was wondering if we could inject the TracingMetadata automatically, so the user doesn't have to do that. We may require to adjust the Message to add the OTel Context as a Metadata automatically if exists. The tricky part may be to extract the Context back again and add it to the right place from a Uni.

@cescoffier @ozangunalp any thoughts?

[codecov-commenter]

Codecov Report

Merging #1678 (11f89b3) into main (46e36d3) will increase coverage by 0.30%.
The diff coverage is 86.80%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1678      +/-   ##
============================================
+ Coverage     77.25%   77.56%   +0.30%     
- Complexity     3466     3472       +6     
============================================
  Files           294      298       +4     
  Lines         11652    11658       +6     
  Branches       1487     1479       -8     
============================================
+ Hits           9002     9042      +40     
+ Misses         1954     1925      -29     
+ Partials        696      691       -5     

Impacted Files	Coverage Δ
...o/smallrye/reactive/messaging/TracingMetadata.java	0.00% <0.00%> (ø)
.../reactive/messaging/kafka/IncomingKafkaRecord.java	97.50% <ø> (+1.75%)	⬆️
...active/messaging/kafka/impl/KafkaRecordHelper.java	78.57% <ø> (-1.99%)	⬇️
...messaging/kafka/impl/ce/KafkaCloudEventHelper.java	82.43% <ø> (-0.32%)	⬇️
...ssaging/amqp/tracing/AmqpMessageTextMapGetter.java	50.00% <50.00%> (ø)
...llrye/reactive/messaging/kafka/KafkaConnector.java	88.54% <50.00%> (-0.94%)	⬇️
...e/messaging/rabbitmq/RabbitMQMessageConverter.java	42.59% <50.00%> (ø)
...ssaging/kafka/tracing/KafkaTraceTextMapGetter.java	53.33% <53.33%> (ø)
...g/rabbitmq/tracing/RabbitMQTraceTextMapGetter.java	53.84% <53.84%> (ø)
...ssaging/amqp/tracing/AmqpMessageTextMapSetter.java	66.66% <66.66%> (ø)
... and 28 more

[ozangunalp]

@radcortez I've not finished reviewing the code but TracingAppToAmqpTest.testFromAppToAmqp is failing

[radcortez]

It seems to be the same issue reported in #1268.

From what I can tell, the 10 messages are produced correctly, but only 9 reach the consumer. I don't think this is related to any of our code, but I also find it strange if the broker is not able to handle this properly.

Do you have any ideas?

[ozangunalp]

@radcortez I looked at the changes again. I noticed that we are doing the same sequence of operations on the instrumenter.

I separated that code into a common module which also imports the otel apis. Here is my test PR : ozangunalp#12

Along the way I think I fixed that flaky test in AMQP connector.
There is something wrong with the RabbitMQ connector tracing config: the tracing.attribute-headers is never taken into account. As we discussed I think the decorator is not needed as it is a connector-specific code and can be added directly to the incoming channel stream.

Tell me what do you think.

[radcortez]

@radcortez I looked at the changes again. I noticed that we are doing the same sequence of operations on the instrument.

Yes, my plan was to refactor this later. Thanks for doing it :)

I separated that code into a common module which also imports the otel apis. Here is my test PR : ozangunalp#12

+1

Along the way I think I fixed that flaky test in AMQP connector. There is something wrong with the RabbitMQ connector tracing config: the tracing.attribute-headers is never taken into account. As we discussed I think the decorator is not needed as it is a connector-specific code and can be added directly to the incoming channel stream.

The tracing.attribute-headers were used in the old implementation, but I think they don't make sense. These allowed to record in the span a configurable set of message headers, but these could clash with the OTel-defined span names, so I think we should remove them.

Tell me what do you think.

+1 Good to go!

[ozangunalp]

@cescoffier I am willing to take this in for 3.23. Could you take a look ?

[cescoffier]

LGTM!