[feat][client] PIP-446: Support Native OpenTelemetry Tracing in Pulsar Java Client #24873
Conversation
Technoboy-
added
ready-to-test
area/client
type/feature
|
@asafm Do you have a chance to review this PR? |
|
@lhotari yes. This was the only comment I saw. |
poorbarcode
left a comment
poorbarcode
left a comment
There was a problem hiding this comment.
I have one suggestion, it should be useful to determine client-side publish latency issue
- Should we add more events for producers, to let us to record more spans? Such as follows
users call send->cache message with message containerpublish message to broker->receive broker response
|
|
||
| @Override | ||
| public void close() { | ||
| // No cleanup needed - spans are attached to messages |
There was a problem hiding this comment.
| // No cleanup needed - spans are attached to messages | |
| // Producer will fail pending messages when it being closed, which will trigger the `onSendAcknowledgement` events |
...nt/src/main/java/org/apache/pulsar/client/impl/tracing/OpenTelemetryProducerInterceptor.java
Show resolved
Hide resolved
Technoboy-
force-pushed
the
client-trace-1
branch
from
2ff2f97 to
c11feac
Compare
lhotari
left a comment
lhotari
left a comment
There was a problem hiding this comment.
Please check the comments about exposing internals on the public API.
| /** | ||
| * Set the OpenTelemetry span associated with this message. | ||
| * <p> | ||
| * This method is called by tracing interceptors to attach a span to the message | ||
| * for later retrieval when completing the span. | ||
| * | ||
| * @param span the span to associate with this message, or null to clear | ||
| */ | ||
| void setTracingSpan(Span span); |
There was a problem hiding this comment.
It doesn't seem to make sense to have this method on the public Pulsar client API. Isn't this a method that should only be used internally?
| * | ||
| * @param span the span to associate with this message ID, or null to clear | ||
| */ | ||
| void setTracingSpan(Span span); |
There was a problem hiding this comment.
It doesn't seem to make sense to have this method on the public Pulsar client API. Isn't this a method that should only be used internally?
| * @param span the span to end | ||
| */ | ||
| public static void endSpan(Span span) { | ||
| if (span != null && span.isRecording()) { |
There was a problem hiding this comment.
the usage of span.isRecording() doesn't make sense here. It should only be used to skip adding some costly values to the span.
| if (span != null && span.isRecording()) { | ||
| span.setStatus(StatusCode.ERROR, throwable.getMessage()); | ||
| span.recordException(throwable); | ||
| span.end(); | ||
| } |
There was a problem hiding this comment.
Please revisit the usage of span.isRecording()
| if (span != null && span.isRecording()) { | |
| span.setStatus(StatusCode.ERROR, throwable.getMessage()); | |
| span.recordException(throwable); | |
| span.end(); | |
| } | |
| if (span != null) { | |
| span.setStatus(StatusCode.ERROR, throwable.getMessage()); | |
| if (span.isRecording() { | |
| span.recordException(throwable); | |
| } | |
| span.end(); | |
| } |
| Message<String> msg = consumer.receive(); | ||
|
|
||
| // Create a custom span for processing | ||
| Span span = tracer.spanBuilder("process-message") | ||
| .setSpanKind(SpanKind.INTERNAL) | ||
| .startSpan(); | ||
|
|
||
| try (Scope scope = span.makeCurrent()) { | ||
| // Your processing logic | ||
| processMessage(msg.getValue()); | ||
| span.setStatus(StatusCode.OK); | ||
| } catch (Exception e) { | ||
| span.recordException(e); | ||
| span.setStatus(StatusCode.ERROR); | ||
| throw e; | ||
| } finally { | ||
| span.end(); | ||
| consumer.acknowledge(msg); | ||
| } |
There was a problem hiding this comment.
I'm just wondering how could this work. Does this PR contains tests that the custom processing span becomes a child span of the span that is created by the PulsarClient tracing.
lhotari
left a comment
lhotari
left a comment
There was a problem hiding this comment.
Please also check the comments about "custom span creation"
| /** | ||
| * Integration tests for OpenTelemetry tracing with real broker. | ||
| * Note: These tests may be timing-dependent and could be flaky in CI environments. | ||
| * They verify end-to-end tracing functionality with actual Pulsar broker. | ||
| */ | ||
| @Test(groups = "broker") | ||
| public class OpenTelemetryTracingIntegrationTest extends BrokerTestBase { |
There was a problem hiding this comment.
This test class doesn't seem to test that "Custom Span Creation" feature described in TRACING.md works as expected. I believe that it would be expected that the custom span would be the child span of the span created by the Pulsar client's consume tracing.
5 participants
Motivation
Implementation of PIP-446
Documentation
docdoc-requireddoc-not-neededdoc-complete