Skip to content

Conversation

@gensericghiro
Copy link
Contributor

@gensericghiro gensericghiro commented Nov 17, 2025

Summary

When working on
KIP-1091,
we mistakenly applied the process-id tag to all client-level metrics,
rather than just the client-state, thread-state, and
recording-level metrics as specified in the KIP. This issue came to
light while working on KIP-1221, which aimed to add the application-id
as a tag to the client-state metric introduced by KIP-1091. This PR
removes these tags from all metrics by default, and adds them to only
the client-state (application-id + process-id) and the
recording-level (process-id only)

Reviewers: Matthias Sax[email protected] Bill
Bejeck[email protected]

Tests

Unit tests in ClientMetricsTest.java and StreamsMetricsImplTest.java

@github-actions github-actions bot added triage PRs from the community streams labels Nov 17, 2025
@bbejeck
Copy link
Member

bbejeck commented Nov 18, 2025

@gensericghiro the failure is related - looks like one test missed the constructor update

@mjsax mjsax added ci-approved and removed triage PRs from the community labels Nov 18, 2025
@mjsax mjsax changed the title KAFKA-19882 Cleaning client level metric tags KAFKA-19882: Cleaning client level metric tags Nov 18, 2025
Copy link
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup. LGTM.

Copy link
Member

@bbejeck bbejeck left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @gensericghiro! LGTM

@bbejeck bbejeck merged commit bb95e3a into apache:trunk Nov 19, 2025
20 checks passed
@bbejeck
Copy link
Member

bbejeck commented Nov 19, 2025

Merged #20906 into trunk

mjsax pushed a commit that referenced this pull request Nov 21, 2025
## Summary
As a follow-up to #20906, also removing the process-id tag from 4.1.
This is because when working on
KIP-1091, we mistakenly applied the process-id tag to all client-level
metrics, rather than just the client-state, thread-state, and
recording-level metrics as specified in the KIP.

## Tests
Unit tests in `ClientMetricsTest.java` and `StreamsMetricsImplTest.java`

Reviewers: Matthias Sax <[email protected]>, Bill Bejeck<[email protected]>
mjsax pushed a commit that referenced this pull request Nov 22, 2025
As a follow-up to #20906, also removing the process-id tag from 4.1.
This is because when working on
KIP-1091, we mistakenly applied the process-id tag to all client-level
metrics, rather than just the client-state, thread-state, and
recording-level metrics as specified in the KIP.

Unit tests in `ClientMetricsTest.java` and `StreamsMetricsImplTest.java`

Reviewers: Matthias Sax <[email protected]>, Bill Bejeck<[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants