-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
remove duplicated broker prometheus metrics type #8995
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.
LGTM
Pattern typePattern = Pattern.compile("^#\\s+TYPE\\s+(\\w+)\\s+(\\w+)"); | ||
Pattern metricNamePattern = Pattern.compile("^(\\w+)\\{.+"); | ||
|
||
Splitter.on("\n").split(metricsStr).forEach(line -> { |
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.
I think this block has intention issue.
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.
We don't use tabs in the code.
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.
Removed the tab. The code block is to catch any issues with duplicated definition of metrics TYPE and also ensures the metric name matches the ones defined in the type definition header.
typeDefs.put(metricName, type); | ||
} else { | ||
fail("Duplicate type definition found for TYPE definition " + metricName); | ||
System.out.println(metricsStr); |
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.
I am not sure why do we need this line if it will fail before this line.
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.
removed
@sijie @wolfstudy can we pick this change to 2.6.3 and 2.7.1 ? |
/pulsarbot run-failure-checks |
### Motivation If there are multiple topics from different namespaces, the broker prometheus metrics will print out duplicated `# TYPE` definition for pulsar_ml_AddEntryBytesRate and other managed ledger metrics. In fact, this problem can be verified by `promtool` https://github.com/prometheus/prometheus#building-from-source On the broker, run this command to check validity of Pulsar broker metric format. `curl localhost:8080/metrics/ | ~/go/bin/promtool check metrics` ### Modifications To prevent duplicated metrics type definition, the definition is now tracked and only printed out once. It leverages the existing metrics name Set already defined under parseMetricsToPrometheusMetrics() in PrometheusMetricsGenerator.java ### Verifying this change - [ x] Make sure that the change passes the CI checks. This change added tests and can be verified as follows: Added two topics under new namespaces to trigger conditions that duplicated prometheus type could happen previously under testManagedLedgerStats() of PrometheusMetricsTest.java. Updated test cases checks this duplicated type problem. (cherry picked from commit 7319819)
### Motivation If there are multiple topics from different namespaces, the broker prometheus metrics will print out duplicated `# TYPE` definition for pulsar_ml_AddEntryBytesRate and other managed ledger metrics. In fact, this problem can be verified by `promtool` https://github.com/prometheus/prometheus#building-from-source On the broker, run this command to check validity of Pulsar broker metric format. `curl localhost:8080/metrics/ | ~/go/bin/promtool check metrics` ### Modifications To prevent duplicated metrics type definition, the definition is now tracked and only printed out once. It leverages the existing metrics name Set already defined under parseMetricsToPrometheusMetrics() in PrometheusMetricsGenerator.java ### Verifying this change - [ x] Make sure that the change passes the CI checks. This change added tests and can be verified as follows: Added two topics under new namespaces to trigger conditions that duplicated prometheus type could happen previously under testManagedLedgerStats() of PrometheusMetricsTest.java. Updated test cases checks this duplicated type problem. (cherry picked from commit 7319819)
Motivation
If there are multiple topics from different namespaces, the broker prometheus metrics will print out duplicated
# TYPE
definition for pulsar_ml_AddEntryBytesRate and other managed ledger metrics.In fact, this problem can be verified by
promtool
https://github.com/prometheus/prometheus#building-from-sourceOn the broker, run this command to check validity of Pulsar broker metric format.
curl localhost:8080/metrics/ | ~/go/bin/promtool check metrics
Modifications
To prevent duplicated metrics type definition, the definition is now tracked and only printed out once. It leverages the existing metrics name Set already defined under parseMetricsToPrometheusMetrics() in PrometheusMetricsGenerator.java
Verifying this change
This change added tests and can be verified as follows:
Added two topics under new namespaces to trigger conditions that duplicated prometheus type could happen previously under testManagedLedgerStats() of PrometheusMetricsTest.java. Updated test cases checks this duplicated type problem.
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesDocumentation