Skip to content

Conversation

@gensericghiro
Copy link
Contributor

@gensericghiro gensericghiro commented Oct 24, 2025

Summary

As a follow-on to the improvements introduced in
KIP-1091
it would be useful to add the application-id as a tag to the
client-state metric. This allows Kafka Streams developers and
operators to connect metrics containing a thread-id (which embeds the
application-id) across separate deployments of Kafka Streams
instances, which are members of the same logical application.

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

@github-actions github-actions bot added streams triage PRs from the community small Small PRs labels Oct 24, 2025
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 PR @gensericghiro! Can you update/add a test to StreamsMetricsImplTest otherwise lgtm

@mjsax mjsax added kip Requires or implements a KIP ci-approved and removed triage PRs from the community labels Oct 24, 2025
@github-actions github-actions bot removed the small Small PRs label Oct 24, 2025
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 @gensericghiro LGTM

@bbejeck bbejeck merged commit b065c60 into apache:trunk Oct 27, 2025
23 checks passed
@bbejeck
Copy link
Member

bbejeck commented Oct 27, 2025

Merged #20766 into trunk

@bbejeck
Copy link
Member

bbejeck commented Oct 27, 2025

@gensericghiro let's do the documentation in a follow-up PR

final Map<String, String> tagMap = new LinkedHashMap<>();
tagMap.put(CLIENT_ID_TAG, clientId);
tagMap.put(PROCESS_ID_TAG, processId);
tagMap.put(APPLICATION_ID_TAG, applicationId);
Copy link
Member

Choose a reason for hiding this comment

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

Pardon me. I see this new tag applied al all metrics, yet the KIP only specifies the client-state metric. Am I misunderstanding the scope? thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, good catch. It doesn't seem like I can add a tag for a specific metric however. Since addClientStateTelemetryMetric is calling the generic addClientLevelMutableMetric method, where tags get added, we'd need to add conditional logic there, which I don't think is something we want.

Copy link
Member

Choose a reason for hiding this comment

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

@chia7712 - thanks for pointing this out. I'm going to start a discussion on the mailing list with a couple of options on how to address the situation.

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

Labels

ci-approved kip Requires or implements a KIP streams

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants