Repository navigation
[jmx-metrics] Additional kafka broker metrics - #19889
breedx-splk wants to merge 8 commits into
Conversation
Pull request dashboard statusWaiting on the author · refreshed 2026-10-13 12:25 UTC Move out of draft to request review. Status above doesn't look right?
|
There was a problem hiding this comment.
Pull request overview
Adds Kafka broker JMX metrics for leadership, ISR/network rates, request latency, and processor/handler utilization.
Changes:
- Adds metric mappings for the new Kafka broker measurements.
- Extends integration verification for metric metadata and attributes.
- Documents the newly exposed metrics.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
KafkaBrokerTest.java |
Verifies all added metrics and attributes. |
experimental-kafka-broker.yaml |
Defines the new Kafka JMX mappings. |
kafka-broker.md |
Documents the expanded metric set. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bf20a42 to
fb3ded8
Compare
|
Before starting to expand Kafka metrics, I think it would be better to make them fit better with semantic conventions, which is the goal of #14279. The current definition has been moved "as-is" without modification so far, but those kafka metrics currently do not fit the JMX recommendations, for example:
If we proceed to merge this PR as-is, it means more breaking changes when #14279 is being implemented. |
Thanks @SylvainJuge for highlighting this.
Moved to where? I don't follow.
What I hear you saying is that if this PR were merged now, we would be generating metrics that might change when we continue the stabilization efforts? Is that the worst thing? I think I understand not wanting to increase the scope during this other change, but is it really that bad to have new/experimental metrics undergo changes? And if so, what's the alternative? Publish these with an explicit experimental prefix or just wait or something else? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
instrumentation/jmx-metrics/library/src/main/resources/jmx/rules/experimental-kafka-broker.yaml:203
- [General] This also silently renames the two existing log-flush metrics, so consumers of
kafka.logs.flush.time.50pand.99pwill lose those series despite the PR being described as additive. Keep these public metric names stable, or document and release the rename as a breaking change perCONTRIBUTING.md.
metric: time.p50
desc: Log flush time - 50th percentile
99thPercentile:
metric: time.p99
|
@trask do you have any preference on whether new metrics should use |
|
my preference would be to hold off on adding any new JMX metrics until the existing JMX metrics are ready for 3.0: |
As far as I can tell kafka metrics are experimental and nothing is done in these PRs to move them closer to stable. Do you want to rename the existing metrics to conform to semconv rules first? If so could this change be done in a minor release as breaking or should it wait for 3.0? |
With #19780 the metrics definitions are being renamed, so it's another good reason to wait until it's merged first before extending metrics. Changing the metrics definitions is technically a breaking change, but it's on something labelled "experimental", so we could do it in a minor (as we did so far). Ideally this kafka metrics alignment should have been done long time ago. I think doing the alignment first would be better, but it's mostly a personal preference:
|
yeah, I think this is ok. my preference is more about prioritizing the JMX work that's needed for 3.0 first to make sure that lands. |
|
I've opened #20570 to improve the current kafka metrics for 3.0, once it's merged we should be able to unblock progress on this PR. |
The reformatting of the markdown table makes it hard to see what has changed, but here are the new entries:
kafka.leader.countkafka.isr.operation.rate.1m,.5m,.15m,.mean(operation)kafka.network.io.rate.1m,.5m,.15m,.mean(direction)kafka.request.total.time.p99(type)kafka.request.queue.time.p99(type)kafka.request.local.time.p99(type)kafka.request.remote.time.p99(type)kafka.request.response.queue.time.p99(type)kafka.request.response.send.time.p99(type)kafka.network.processor.idle.averagekafka.network.processor.idle.utilization(kafka.network.processor.id)kafka.request.handler.idle.meankafka.request.handler.idle.1mExisting Kafka percentile metrics retain their original names (
kafka.request.time.50p,kafka.request.time.99p,kafka.logs.flush.time.50p, andkafka.logs.flush.time.99p) to avoid breaking changes. The new percentile metrics use the recommended.p99suffix.(based on some prior art from over here, codex assisted)