messaging.kafka.bootstrap.servers added for KafkaProducer#17065
messaging.kafka.bootstrap.servers added for KafkaProducer#17065trask merged 21 commits intoopen-telemetry:mainfrom
Conversation
onurkybsi
force-pushed
the
kafka-bootstrap-servers
branch
from
6484a3b to
d125e22
Compare
| if (request.getBootstrapServers() != null) { | ||
| attributes.put(MESSAGING_KAFKA_BOOTSTRAP_SERVERS, request.getBootstrapServers()); |
There was a problem hiding this comment.
attributes.put ignores null values so null check isn't needed
our convention is that we don't emit non-spec attributes in default configuration. For example see https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/kafka/kafka-clients/kafka-clients-common-0.11/library/src/main/java/io/opentelemetry/instrumentation/kafkaclients/common/v0_11/internal/KafkaConsumerExperimentalAttributesExtractor.java
|
|
||
| /** Sets Kafka consumer configurations. */ | ||
| @CanIgnoreReturnValue | ||
| public KafkaTelemetryBuilder setProducerConfigs(Map<String, Object> producerConfigs) { |
There was a problem hiding this comment.
this is not ideal, currently the same KafkaTelemetry instance could be used to add telemetry to different producers but with this it is not the case any more. Did you consider alternatives like using reflection to read the producerConfig field in KafkaProducer?
There was a problem hiding this comment.
Hey @laurit, thanks for the review, it's done!
onurkybsi
force-pushed
the
kafka-bootstrap-servers
branch
2 times, most recently
from
f8727e0 to
6514ffb
Compare
|
hey @onurkybsi! don't be alarmed, I just pushed 8d1a604 to your branch to align with some repo test conventions and bring the diff line count down a bit in the process |
onurkybsi
force-pushed
the
kafka-bootstrap-servers
branch
2 times, most recently
from
752f542 to
02aef9f
Compare
# Conflicts: # instrumentation/kafka/kafka-clients/kafka-clients-2.6/library/src/main/java/io/opentelemetry/instrumentation/kafkaclients/v2_6/KafkaTelemetryBuilder.java # Conflicts: # instrumentation/spring/spring-kafka-2.7/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/spring/kafka/v2_7/SpringKafkaTest.java # Conflicts: # instrumentation/kafka/kafka-clients/kafka-clients-2.6/library/src/main/java/io/opentelemetry/instrumentation/kafkaclients/v2_6/internal/KafkaProducerTelemetry.java
onurkybsi
force-pushed
the
kafka-bootstrap-servers
branch
from
327fa0b to
4c2d083
Compare
| implements AttributesExtractor<KafkaProducerRequest, RecordMetadata> { | ||
|
|
||
| private static final AttributeKey<String> MESSAGING_KAFKA_BOOTSTRAP_SERVERS = | ||
| AttributeKey.stringKey("messaging.kafka.bootstrap.servers"); |
There was a problem hiding this comment.
wondering whether stringArrayKey would be a better fit as we really have a list of bootstrap servers
@trask do you have any guidance on this
| equalTo(MESSAGING_KAFKA_MESSAGE_OFFSET, 0), | ||
| equalTo( | ||
| stringKey("messaging.kafka.bootstrap.servers"), | ||
| isExperimental ? kafka.getBootstrapServers() : null)); |
There was a problem hiding this comment.
Hey @laurit, fyi, I was getting warning since equalTo parameters are not nullable, that's why I had made my change.
There was a problem hiding this comment.
nobody has gotten around to submitting a PR that adds the @Nullable annotation there
…vers # Conflicts: # instrumentation/kafka/kafka-clients/kafka-clients-2.6/library/src/test/java/io/opentelemetry/instrumentation/kafkaclients/v2_6/InterceptorsTest.java # instrumentation/kafka/kafka-clients/kafka-clients-2.6/library/src/test/java/io/opentelemetry/instrumentation/kafkaclients/v2_6/WrapperSuppressReceiveSpansTest.java # instrumentation/kafka/kafka-clients/kafka-clients-2.6/library/src/test/java/io/opentelemetry/instrumentation/kafkaclients/v2_6/WrapperTest.java
|
Thank you for your contribution @onurkybsi! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
3 participants
messaging.kafka.bootstrap.serversattribute added for instrumentedKafkaProducer, see #10647.