Pulsar: use span links when receive telemetry is enabled #10650
Conversation
| @@ -87,24 +99,28 @@ private static Instrumenter<PulsarBatchRequest, Void> createConsumerBatchReceive | |||
| createMessagingAttributesExtractor(getter, MessageOperation.RECEIVE)) | |||
| .addAttributesExtractor( | |||
| ServerAttributesExtractor.create(new PulsarNetClientAttributesGetter())) | |||
| .setEnabled(ExperimentalConfig.get().messagingReceiveInstrumentationEnabled()) | |||
There was a problem hiding this comment.
spring kafka instrumentation also produces telemetry with span links for the batch receive scenario that can't be modeled with just parent child relationships
| span -> | ||
| span.hasName(topic + " receive") | ||
| .hasKind(SpanKind.CONSUMER) | ||
| .hasParent(trace.getSpan(1)) | ||
| .hasNoParent() | ||
| .hasLinks(LinkData.create(producerSpan.get().getSpanContext())) |
There was a problem hiding this comment.
I think this deviates a bit from kafka, here we place the span link on both receive and process spans but kafka places it only on the process span.
...io/opentelemetry/javaagent/instrumentation/pulsar/v2_8/telemetry/MessageListenerContext.java
Outdated
Show resolved
Hide resolved
...io/opentelemetry/javaagent/instrumentation/pulsar/v2_8/telemetry/MessageListenerContext.java
Outdated
Show resolved
Hide resolved
.../java/io/opentelemetry/javaagent/instrumentation/pulsar/v2_8/telemetry/PulsarSingletons.java
Outdated
Show resolved
Hide resolved
.../java/io/opentelemetry/javaagent/instrumentation/pulsar/v2_8/telemetry/PulsarSingletons.java
Show resolved
Hide resolved
| List<AttributeAssertion> assertions = | ||
| new ArrayList<>( | ||
| Arrays.asList( | ||
| equalTo(SemanticAttributes.MESSAGING_SYSTEM, "pulsar"), |
There was a problem hiding this comment.
Static imports for improved readability.
| .hasParent(trace.getSpan(1)))); | ||
| } | ||
|
|
||
| static List<AttributeAssertion> sendAttributes( |
There was a problem hiding this comment.
The method name doesn't match the behavior. Maybe something more like makeSendAttributeAssertions() or even sendAttrAssertions() or something.
There was a problem hiding this comment.
The same pattern is used in kafka tests. In my opinion it is fine, these methods are only used in tests and attributes vs attribute assertions isn't a big difference.
| return receiveAttributes(destination, messageId, testHeaders, false); | ||
| } | ||
|
|
||
| static List<AttributeAssertion> receiveAttributes( |
There was a problem hiding this comment.
Same comment about the name. Maybe makeReceiveAttrAssertions().
...st/java/io/opentelemetry/javaagent/instrumentation/pulsar/v2_8/AbstractPulsarClientTest.java
Show resolved
Hide resolved
...pentelemetry/javaagent/instrumentation/pulsar/v2_8/PulsarClientSuppressReceiveSpansTest.java
Outdated
Show resolved
Hide resolved
| consumer = | ||
| client | ||
| .newConsumer(Schema.STRING) | ||
| .subscriptionName("test_sub") | ||
| .topic(topic) | ||
| .subscriptionInitialPosition(SubscriptionInitialPosition.Earliest) | ||
| .subscribe(); |
There was a problem hiding this comment.
Wait, where does the "receive with timeout" part factor into this test? I'm not seeing it. If it's there, perhaps a comment can help to clarify, because a reader doesn't immediately see how the receive-with-timeout is wired up.
There was a problem hiding this comment.
I presume it is consumer.receive(1, TimeUnit.MINUTES);
...t/src/test/java/io/opentelemetry/javaagent/instrumentation/pulsar/v2_8/PulsarClientTest.java
Show resolved
Hide resolved
Resolves #8859
Supersedes #8909
This PR alters spans produced by pulsar instrumentation to behave similarly to kafka instrumentation. If
receive-telemetryis enabled place producer and consumer spans in different traces that are connected via span links. Ifreceive-telemetryis not enabled place producer and consumer in the same trace and don't createreceivespan if the trace contains aprocessspan.