-
-
Notifications
You must be signed in to change notification settings - Fork 221
issues/1506: ensure Prometheus /metrics endpoint streams correctly wi… #1536
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi chris-wood-mo! 👋
Welcome, and thank you for opening your first PR in the repo!
Please wait for triaging by our maintainers.
Please take a look at our contributing guide.
api/src/main/java/io/kafbat/ui/controller/PrometheusExposeController.java
Outdated
Show resolved
Hide resolved
… and updated response of exposeClusterMetrics and exposeAllMetrics to be string
|
Hi @germanosin, Thanks for your comments! I have reviewed them and updated the code. I have also made sure to test the changes again and make sure they work both locally and from a Kubernetes cluster using a service monitor to scrape the metrics into Prometheus. Both have worked with the changes made. The screenshot below shows this working locally: Thanks, Chris |
germanosin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it should be changed here?
https://github.com/kafbat/kafka-ui/blob/main/api/src/main/java/io/kafbat/ui/service/metrics/prometheus/PrometheusMetricsExposer.java#L33C86-L33C98
Are you sure that this is not an openmetrics format?
…t content-type for Prometheus text format
…t content-type for Prometheus text format
|
@germanosin The previous change did not return an openmetrics format, but you make a really good point of the location of the change! I have made the amendment you have suggested. Again I have made sure to check that this works both locally and from a Kubernetes cluster. I can also confirm that is returning the correct content type too: Content Type: The screenshot below shows this working locally: Thanks Chris |
|
@chris-wood-mo thanks for contribution, It is in the main branch with test now, was fixed with another issue. |
|
Hey @germanosin, Thanks for including that, I really appreciate that change being added to the main branch so quickly! Thanks for all the hard work you're doing on this project! |


What changes did you make? (Give an overview)
Is there anything you'd like reviewers to focus on?
How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)
As you can see from the evidence attached, this now works locally and returns the format expected by Prometheus.
Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)
Check out Contributing and Code of Conduct
A picture of a cute animal (not mandatory but encouraged)
