feat(otel): Add OpenTelemetry distributed tracing support#1769
feat(otel): Add OpenTelemetry distributed tracing support#1769jbonofre wants to merge 1 commit intoapache:mainfrom
Conversation
jbonofre
changed the title
New activemq-opentelemetry module that instruments the broker using the OpenTelemetry API. The plugin traces send (PRODUCER), dispatch (CONSUMER), and acknowledge (INTERNAL) operations with W3C TraceContext propagation and standard OTel messaging semantic conventions. Depends on opentelemetry-api only; users bring their own SDK and exporter at runtime. Included in the distribution with example configuration.
jbonofre
force-pushed
the
add-opentelemetry-support
branch
from
0e1e69d to
176f5f5
Compare
jeanouii
left a comment
jeanouii
left a comment
There was a problem hiding this comment.
Great PR and a good start.
I've added a couple of notes and questions.
It's been a while since I worked with this.
| @Override | ||
| public void postProcessDispatch(MessageDispatch messageDispatch) { | ||
| if (messageDispatch != null) { | ||
| Span span = dispatchSpans.remove(messageDispatch); |
There was a problem hiding this comment.
Because by nature, this can't be done in a try/finally block, some spans may leak in the map forever.
The map needs to be either bound or we need to implement a TTL to remove old entries in my opinion.
| } | ||
| if (producerExchange.getConnectionContext() != null | ||
| && producerExchange.getConnectionContext().getClientId() != null) { | ||
| spanBuilder.setAttribute("messaging.client_id", producerExchange.getConnectionContext().getClientId()); |
There was a problem hiding this comment.
We use client_id a lot, but naming conventions require . (dot)
| SpanBuilder spanBuilder = tracer.spanBuilder(destinationName + " publish") | ||
| .setSpanKind(SpanKind.PRODUCER) | ||
| .setParent(parentContext) | ||
| .setAttribute("messaging.system", "activemq") |
There was a problem hiding this comment.
"messaging.system" --> "messaging.system.name"?
| .setParent(parentContext) | ||
| .setAttribute("messaging.system", "activemq") | ||
| .setAttribute("messaging.destination.name", destinationName) | ||
| .setAttribute("messaging.operation", "publish"); |
There was a problem hiding this comment.
"messaging.operation" --> "messaging.operation.type"?
| } | ||
|
|
||
| TextMapPropagator propagator = GlobalOpenTelemetry.getPropagators().getTextMapPropagator(); | ||
| Tracer tracer = GlobalOpenTelemetry.getTracer(INSTRUMENTATION_NAME); |
There was a problem hiding this comment.
Shouldn't these 2 lines be done in the constructor only and not for each invocation?
| public void set(Message message, String key, String value) { | ||
| try { | ||
| message.setProperty(key, value); | ||
| message.setMarshalledProperties(null); |
There was a problem hiding this comment.
This is to force the properties to be injected, correct?
what's the performance impact?
| producer.close(); | ||
|
|
||
| // Allow async broker processing to complete | ||
| Thread.sleep(500); |
There was a problem hiding this comment.
Can you use the WaitFor pattern instead?
| Context extractedContext = propagator.extract(Context.current(), message, ActiveMQMessageTextMapGetter.INSTANCE); | ||
|
|
||
| String destinationName = message.getDestination().getPhysicalName(); | ||
| SpanBuilder spanBuilder = tracer.spanBuilder(destinationName + " deliver") |
There was a problem hiding this comment.
How is this going to show up for topics?
Many spans for the same message?
There was a problem hiding this comment.
Yes, it's multiple messages per topic here.
2 participants
New activemq-opentelemetry module that instruments the broker using the OpenTelemetry API. The plugin traces send (PRODUCER), dispatch (CONSUMER), and acknowledge (INTERNAL) operations with W3C TraceContext propagation and standard OTel messaging semantic conventions.
Depends on opentelemetry-api only; users bring their own SDK and exporter at runtime. Included in the distribution with example configuration. Maybe we should add a default SDK/exporter in the ActiveMQ distribution, just wondering 😄