Skip to content
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.

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

Fix units in the Kafka metric semantic conventions #3300

Merged
merged 6 commits into from
Mar 29, 2023

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Mar 7, 2023

  • The correct UCUM case-sensitive code for bytes is By not by
  • Change the lag max annotation to report what the description describes it measuring, namely messages.

@MrAlias MrAlias requested review from a team March 7, 2023 01:07
Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

Next time you touch these semconv, can you move them to YAML?

@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 7, 2023

Next time you touch these semconv, can you move them to YAML?

We should probably create an issue to address this (and the other metric semantic conventions). I'm happy to add it next time, but I don't plan to touch this again in the foreseeable future.

@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 7, 2023

Next time you touch these semconv, can you move them to YAML?

We should probably create an issue to address this (and the other metric semantic conventions). I'm happy to add it next time, but I don't plan to touch this again in the foreseeable future.

@trask is there already an issue tracking this work?

@trask
Copy link
Member

trask commented Mar 7, 2023

@trask is there already an issue tracking this work?

looks like there's two already 😀 #2001 and #2666

Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

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

Approve this change, but what instrumentation actually produces this telemetry? My understanding is that this data is produced / exposed by kafka brokers, and that its bridged into the opentelemetry collector via the collector jmxreceiver which uses the java jmx-metrics which defines the mapping in kafka.groovy, kafka-consumer.groovy, and kafka-producer.groovy.

If this is in fact the case, then there's no actually instrumentation at the source (i.e. in the kafka brokers themselves) that produces these metrics. For this change to have any change, the maintainers of java jmx-metrics would need to be aware of this change and go update the code to reflect the updates. This seems unlikely to happen as there's very little activity in the jmx-metrics module, and the maintainers aren't involved in these conversations about semantic conventions.

I want to rearticulate the point I made here that we should not try to codify the mapping rules of bridge components like jmx-metrics into semantic conventions. I think we should delete this document.

@pyohannes
Copy link
Contributor

If this is in fact the case, then there's no actually instrumentation at the source (i.e. in the kafka brokers themselves) that produces these metrics. For this change to have any change, the maintainers of java jmx-metrics would need to be aware of this change and go update the code to reflect the updates. This seems unlikely to happen as there's very little activity in the jmx-metrics module, and the maintainers aren't involved in these conversations about semantic conventions.

I want to rearticulate the point I made #2610 (comment) that we should not try to codify the mapping rules of bridge components like jmx-metrics into semantic conventions. I think we should delete this document.

I agree, I added some similar thoughts in a comment in #2485 (comment), referencing efforts in the Kafka community to directly expose OTel metrics via the Kafka broker.

I think this document in its current form will not survive any stabilization efforts. There will be generic semantic conventions for metrics emitted by message producers and consumers, however, I'm in doubt whether it makes sense to have semantic conventions pertaining to broker-specific instrumentation. Ideally, those would be owned and maintained by the respective community (Kafka developers), as I think the OTel community will not have the bandwidth to maintain a wide range of such specific conventions.

Regarding deleting this document, I think the intention was to keep those JMX specific conventions around until we come up with something better (#2485 (comment)).

@jsuereth
Copy link
Contributor

I want to rearticulate the point I made #2610 (comment) that we should not try to codify the mapping rules of bridge components like jmx-metrics into semantic conventions. I think we should delete this document.

Totally agree. Let's first fix the units as part of this PR and we can discuss the meta-issue of semconv for "bridge" / "pre-existing instrumentation" as a different issue.

@arminru arminru added semconv:messaging area:semantic-conventions Related to semantic conventions labels Mar 27, 2023
@arminru arminru merged commit 9352c25 into open-telemetry:main Mar 29, 2023
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions semconv:messaging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants