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: Confluent Platform Intergration - Correct Mbean regex stream thread metric #17490

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

tlsdnwn55
Copy link

What does this PR do?

Fixed the JMX Mbean regex to collect stream thread metrics of ksqldb in Confluent Platform.
And added the blocked-time-ns-total metric. (described in confluent docs as being very useful for debugging Kafka Streams application performance)

Motivation

I am maintaining a confluent platform, and while configuring the ksqldb dashboard, I discovered that stream thread metrics were not collected.

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Changelog entries must be created for modifications to shipped code
  • Add the qa/skip-qa label if the PR doesn't need to be tested during QA.
  • If you need to backport this PR to another branch, you can add the backport/<branch-name> label to the PR and it will automatically open a backport PR once this one is merged

@tlsdnwn55 tlsdnwn55 requested a review from a team as a code owner May 1, 2024 00:14
@datadog-agent-integrations-bot datadog-agent-integrations-bot bot added documentation qa/skip-qa Automatically skip this PR for the next QA team/documentation labels May 1, 2024
Copy link

codecov bot commented May 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.83%. Comparing base (2a6ec26) to head (4bc637c).
Report is 105 commits behind head on master.

Additional details and impacted files
Flag Coverage Δ
confluent_platform 81.96% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link

github-actions bot commented May 1, 2024

Test Results

8 files  8 suites   53s ⏱️
1 tests 1 ✅ 0 💤 0 ❌
8 runs  4 ✅ 4 💤 0 ❌

Results for commit d07f286.

♻️ This comment has been updated with latest results.

jhgilbert
jhgilbert previously approved these changes May 1, 2024
@tlsdnwn55
Copy link
Author

@jhgilbert thank you for your approve.

@tlsdnwn55
Copy link
Author

@jhgilbert how may i merge this pr?

- include:
domain: kafka.streams
bean_regex: kafka\.streams:type=stream-metrics,client-id=.*
bean_regex: kafka\.streams:type=stream-thread-metrics,thread-id=.*
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add this entry as new one for this bean regex instead of overwriting the current one? If we overwrite this, it might break some backwards compatibility with at least version 5 of confluent platform:
https://docs.confluent.io/legacy/platform/5.0.0/streams/monitoring.html#:~:text=MBean%3A%20kafka.streams%3Atype%3Dstream%2Dmetrics%2Cthread.client%2Did%3D%5BthreadId%5D

Moving this to a new entry will ensure both legacy and new versions to submit this metric.

Copy link
Author

Choose a reason for hiding this comment

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

@steveny91 Thank you your advice.
Metrics have been added for compatibility up to version 5.4.10.
Please confirm.

Copy link
Contributor

@steveny91 steveny91 left a comment

Choose a reason for hiding this comment

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

@tlsdnwn55 Thanks for the contribution. I left one comment on the change.

@tlsdnwn55 tlsdnwn55 requested a review from steveny91 May 10, 2024 00:38
@tlsdnwn55
Copy link
Author

@steveny91 Could you please confirm?

@tlsdnwn55 tlsdnwn55 requested a review from jhgilbert May 23, 2024 12:35
@@ -0,0 +1,2 @@

Fix correct Mbean regex stream thread metric(after version 5.5.0 of confluent platform). Previously, because Bean Regex was set to Client metrics, Stream Thread Metrics were not collected. also added one useful attribute for debugging Stream Thread performance.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Fix correct Mbean regex stream thread metric(after version 5.5.0 of confluent platform). Previously, because Bean Regex was set to Client metrics, Stream Thread Metrics were not collected. also added one useful attribute for debugging Stream Thread performance.
Add a Bean regex for stream thread metrics for Confluent version >=5.5.0. Previously, Stream Thread Metrics were not collected. Also added one useful attribute for debugging stream thread performance.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks I think your suggestion is more appropriate.

steveny91
steveny91 previously approved these changes May 23, 2024
@steveny91
Copy link
Contributor

Looks good on my end. Thanks for the contribution. I'll get this merged in next week so that it can be released with the next agent (7.55).

One last nit is to rename the changelog enty file from .fixed to .added. But I can take care of that as well next week. Thanks again!

@tlsdnwn55
Copy link
Author

@steveny91 thank you!
renamed the changelog to be more appropriate as suggested.
Thank you again for accepting my PR.

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.

None yet

3 participants