Instrument spring-kafka batch message listeners #3922
Conversation
mateuszrzeszutek
requested review from
anuraaga,
iNikem,
jkwatson,
laurit,
pavolloffay,
trask and
tylerbenson
as code owners
| // this instrumentation will create CONSUMER receive spans for each record instead of | ||
| // kafka-clients | ||
| if (it instanceof KafkaConsumerIteratorWrapper) { | ||
| it = ((KafkaConsumerIteratorWrapper<K, V>) it).unwrap(); |
There was a problem hiding this comment.
It might be better to somehow completely suppress kafka-client instrumentation. Currently it is still possible to have kafka-client create extra spans. Copy logback.xml from spring-webmvc-3.1 to test/resources and add <logger name="org.springframework.kafka.listener" level="trace"/>
There was a problem hiding this comment.
It definitely is better to have it disabled by default. For a broader picture, I'm planning to instrument the most commonly used "higher-level" kafka consumer APIs (spring, vertx, reactor, ... anything else?) and after they all have their own separate instrumentations I believe we should have the kafka-clients consumer one disabled by default (which is exactly what @trask suggested in #1947 (comment)). It might take some time though 😅
For now this hack must suffice, but I'd love to turn off the kafka-clients instrumentation completely sometime.
...api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java
Outdated
Show resolved
Hide resolved
| /** Extractor of span links for a request. */ | ||
| @FunctionalInterface | ||
| public interface SpanLinksExtractor<REQUEST> { | ||
|
|
||
| /** | ||
| * Extracts {@link SpanContext}s that should be linked to the newly created span and adds them to | ||
| * {@code spanLinks}. | ||
| */ | ||
| void extract(SpanLinksBuilder spanLinks, Context parentContext, REQUEST request); | ||
|
|
||
| /** | ||
| * Returns a new {@link SpanLinksExtractor} that will extract a {@link SpanContext} from the | ||
| * request using configured {@code propagators}. | ||
| */ | ||
| static <REQUEST> SpanLinksExtractor<REQUEST> fromUpstreamRequest( | ||
| ContextPropagators propagators, TextMapGetter<REQUEST> getter) { | ||
| return new PropagatorBasedSpanLinksExtractor<>(propagators, getter); | ||
| } | ||
| } |
There was a problem hiding this comment.
cc: @anuraaga new Instrumenter API addition
|
|
||
| Context parentContext = Context.current(); | ||
|
|
||
| // create spans for all records received in a batch |
There was a problem hiding this comment.
any particular thoughts on this vs a single receive span with the links on the receive span?
There was a problem hiding this comment.
I tried matching the code with one of the spec example scenarios, I chose the "batch processing" one because it made the most sense to have a single process span for all received messages. It's not a perfect match though - in case of Kafka we're both receiving and processing in batch; so it'd make much more sense to have one receive span that spans the whole consumer.poll() call and a second process span for the spring message listener that links the PRODUCER spans. That's more or less what open-telemetry/opentelemetry-specification#1085 proposes; I guess this PR might be a good reason to revisit the messaging conventions and think what telemetry we should generate in a "batch receive+process" scenario.
…tion/api/instrumenter/InstrumenterBuilder.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
mateuszrzeszutek
force-pushed
the
spring-kafka-batch
branch
from
4b16d91
to
5a3bc8f
Compare
|
Do you mind if I implement the "one receive span + one process span" idea in a separate PR? I thought a bit about how to do this, and it'd probably require lots of changes in this and the kafka-clients instrumentations. I don't really want to make this PR any bigger, it's large enough as it is, so I thought it may be a better idea to implement this thing separately. |
sounds good 👍 |
Fixes #3529