Skip to content

feat(kafka-clients): add messaging.kafka.bootstrap.servers attribute #14032

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

feat(kafka-clients): add messaging.kafka.bootstrap.servers attribute #14032

wants to merge 12 commits into from

Conversation

wl2027
Copy link
Contributor

@wl2027 wl2027 commented Jun 13, 2025 •
edited
Loading

  • Java agent instrumentation obtains bootstrap.servers through constructor instrumentation
  • Library instrumentation obtains bootstrap.servers through reflection and kafka-clients interceptors
  • Use VirtualField for managing bootstrap servers storage
  • Include bootstrap.servers attribute in all relevant spans

#14031 #10647

wl2027 and others added 2 commits June 13, 2025 21:52
@wl2027
feat(kafka-clients): add messaging.kafka.bootstrap.servers attribute
f5f0070 
- Add VirtualFieldStore for managing bootstrap servers storage
- Instrument KafkaConsumer and KafkaProducer constructors
- Include bootstrap.servers attribute in all relevant spans

Addresses part of open-telemetry#14031 open-telemetry#10647
@wl2027
Merge branch 'open-telemetry:main' into kafka-client
f0b98c8
@wl2027 wl2027 requested a review from a team as a code owner June 13, 2025 14:07
@github-actions github-actions bot added the test native This label can be applied to PRs to trigger them to run native tests label Jun 13, 2025
wl2027 and others added 3 commits June 14, 2025 16:55
@otelbot-java-instrumentation
Copy link
Contributor

🔧 The result from spotlessApply was committed to the PR branch.

@wl2027
Copy link
Contributor Author

wl2027 commented Jun 16, 2025

Hi @laurit , Could you review this PR? Thanks!

@breedx-splk
Copy link
Contributor

Thanks! It would be nice if the new attribute would be proposed in https://github.com/open-telemetry/semantic-conventions/blob/main/docs/messaging/kafka.md as well...just sayin'.

breedx-splk
breedx-splk reviewed Jun 16, 2025
View reviewed changes
...opentelemetry/javaagent/instrumentation/kafkaclients/v0_11/KafkaProducerInstrumentation.java Outdated
if (configs instanceof Map) {
Object servers = ((Map<?, ?>) configs).get(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG);
if (servers != null) {
bootstrapServers = servers.toString();
Copy link
Contributor

@breedx-splk breedx-splk Jun 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be an obvious question -- but it also speaks to how backends may want to use these bootstrap servers: should we have the attribute type be a list of strings instead of a comma separated string? The comma separated could imply more work over time for some backends, but I have no idea how it's intended on being used/indexed/searched/whatever.

Copy link
Contributor Author

@wl2027 wl2027 Jun 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I will uniformly convert it from string to list.

@wl2027 @breedx-splk
fix
07e095d 
Co-authored-by: jason plumb <75337021+breedx-splk@users.noreply.github.com>
@wl2027
Copy link
Contributor Author

wl2027 commented Jun 17, 2025 •
edited
Loading

Thanks! It would be nice if the new attribute would be proposed in https://github.com/open-telemetry/semantic-conventions/blob/main/docs/messaging/kafka.md as well...just sayin'.

Thanks for the suggestion! I will first place this attribute in ExperimentalAttributesExtractor, and then create a separate PR to the semantic-conventions repository to propose this new attribute. This ensures standardization for implementations in other languages. However, I'm unsure whether other languages also find it difficult to obtain kafka-clients' server.* and network.* attributes, leading them to implement and use messaging.kafka.bootstrap.servers.

laurit
laurit reviewed Jun 17, 2025
View reviewed changes
...opentelemetry/javaagent/instrumentation/kafkaclients/v0_11/KafkaConsumerInstrumentation.java Outdated
if (servers != null) {
bootstrapServers = servers.toString();
}
} else if (configs instanceof Properties) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this branch is unreachable, java.util.Properties is a subtype of java.util.Map

laurit
laurit reviewed Jun 17, 2025
View reviewed changes
...opentelemetry/javaagent/instrumentation/kafkaclients/v0_11/KafkaConsumerInstrumentation.java Outdated
Comment on lines 82 to 83
VirtualField<Consumer<?, ?>, String> consumerStringVirtualField =
VirtualField.find(Consumer.class, String.class);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move virtual field access to a helper class (for example KafkaSingletons) and keep the VirtualField instance in a static field

laurit
laurit reviewed Jun 17, 2025
View reviewed changes
...opentelemetry/javaagent/instrumentation/kafkaclients/v0_11/KafkaConsumerInstrumentation.java Outdated
@@ -38,6 +43,14 @@ public ElementMatcher<TypeDescription> typeMatcher() {

@Override
public void transform(TypeTransformer transformer) {
transformer.applyAdviceToMethod(
isConstructor().and(takesArgument(0, Map.class)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could use isConstructor().and(takesArgument(0, Map.class).or(takesArgument(0, Properties.class))) to combine the advices

laurit
laurit reviewed Jun 17, 2025
View reviewed changes
...try/instrumentation/kafkaclients/common/v0_11/internal/KafkaConsumerAttributesExtractor.java Outdated
Comment on lines 55 to 58
String bootstrapServers = request.getBootstrapServers();
if (bootstrapServers != null) {
attributes.put(MESSAGING_KAFKA_BOOTSTRAP_SERVERS, bootstrapServers);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default we emit only attributes that are in spec. Either get it added to spec or move to KafkaConsumerExperimentalAttributesExtractor

Copy link
Contributor Author

@wl2027 wl2027 Jun 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I will create a new KafkaExperimentalAttributesExtractor to include this attribute. Additionally, I will extract a parent class AbstractKafkaRequest from xxxRequest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurit laurit laurit left review comments

@breedx-splk breedx-splk breedx-splk left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
test native This label can be applied to PRs to trigger them to run native tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wl2027 @breedx-splk @laurit