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][monitor] Set correct content-type for metrics endpoints in Pulsar for Prometheus 3.x compatibility #24065

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Mar 7, 2025

Motivation

Follow up on #24060 which set an incorrect content type.

Metrics endpoint uses version 1.0.0 of the Prometheus/OpenMetrics text format.

This content-type value ensures compatibility with Prometheus 3.x and later versions.
For details, refer to the Prometheus 3.x migration guide:
https://prometheus.io/docs/prometheus/latest/migration/#scrape-protocols

The difference between version 1.0.0 and 0.0.4 formats is mainly about counters.
The prometheus client_java library >=0.10.0 creates OpenMetrics compatible counters which are not compatible with the 0.0.4 format which is currently in use in BookKeeper.

For implementation details of Prometheus client_java, see:
https://github.com/prometheus/client_java/blob/parent0.16.0/simpleclient/src/main/java/io/prometheus/client/Counter.java#L76-L80
The library will always append "_total" to the counter name unless the counter name already contains "_total" suffix and will have a separate "_created" counter to ensure OpenMetrics compatibility. This change was introduced in prometheus/client_java#615 in version 0.10.0.
Release notes: https://github.com/prometheus/client_java/releases/tag/parent-0.10.0

In Pulsar, the Prometheus client_java library was updated to 0.15.0 with #13785 in Pulsar 2.11.0.

Modifications

  • set version to 1.0.0 in the content type so that scraping will result in correct metrics

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@lhotari
Copy link
Member Author

lhotari commented Mar 8, 2025

The content type might have to be configurable. I'm still unsure whether 1.0.0 is correct or do we need to switch to use application/openmetrics-text; version=1.0.0; charset=utf-8 as specified in https://github.com/prometheus/OpenMetrics/blob/main/specification/OpenMetrics.md#overall-structure

@codecov-commenter
Copy link

codecov-commenter commented Mar 8, 2025

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 74.26%. Comparing base (bbc6224) to head (c71d4de).
Report is 952 commits behind head on master.

Files with missing lines Patch % Lines
...ions/worker/rest/api/FunctionsMetricsResource.java 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #24065      +/-   ##
============================================
+ Coverage     73.57%   74.26%   +0.69%     
+ Complexity    32624    32442     -182     
============================================
  Files          1877     1862      -15     
  Lines        139502   144226    +4724     
  Branches      15299    16434    +1135     
============================================
+ Hits         102638   107112    +4474     
+ Misses        28908    28681     -227     
- Partials       7956     8433     +477     
Flag Coverage Δ
inttests 26.69% <0.00%> (+2.10%) ⬆️
systests 23.11% <0.00%> (-1.22%) ⬇️
unittests 73.78% <66.66%> (+0.93%) ⬆️

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

Files with missing lines Coverage Δ
...ker/stats/prometheus/PrometheusMetricsServlet.java 52.56% <100.00%> (-16.49%) ⬇️
...ats/prometheus/PulsarPrometheusMetricsServlet.java 50.00% <100.00%> (-50.00%) ⬇️
...ions/worker/rest/api/FunctionsMetricsResource.java 0.00% <0.00%> (ø)

... and 1060 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@eolivelli
Copy link
Contributor

+1 to make it configurable, but to allowing tweaks. Let's not change the way we output metrics depending on the overridden value

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