fix prometheus metric provider bug and add test to cover label scope … #2718
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…and metric format check
codelipenghui
approved these changes
May 24, 2021
|
Lgtm |
jiazhai
approved these changes
May 24, 2021
merlimat
approved these changes
May 24, 2021
dlg99
approved these changes
May 24, 2021
Contributor
dlg99
left a comment
There was a problem hiding this comment.
Overall LGTM.
One question regarding need to nullcheck opStat.getLabels()
| .append("\", "); | ||
| writeLabelsNoBraces(w, opStat.getLabels()); | ||
| .append("\""); | ||
| if (!opStat.getLabels().isEmpty()) { |
Contributor
There was a problem hiding this comment.
is it guaranteed that opStat.getLabels() is never null?
Contributor
There was a problem hiding this comment.
Yes, it will return an empty map when there are no labels.
Contributor
Author
There was a problem hiding this comment.
// PrometheusMetricsProvider.java
public StatsLogger getStatsLogger(String scope) {
return new PrometheusStatsLogger(PrometheusMetricsProvider.this, scope, Collections.emptyMap());
}
// PrometheusStatsLogger.java
PrometheusStatsLogger(PrometheusMetricsProvider provider, String scope, Map<String, String> labels) {
this.provider = provider;
this.scope = scope;
this.labels = labels;
}
@Override
public OpStatsLogger getOpStatsLogger(String name) {
return provider.opStats.computeIfAbsent(scopeContext(name), x -> new DataSketchesOpStatsLogger(labels));
}Yes, it will use Collections.emptyMap() to init labels when there are no labels.
Contributor
Author
|
Whether we need to include this PR in 4.14.0 release before Pulsar 2.8.0 release? |
Contributor
|
Unfortunately 4.14.0 is already completed. Probably we must cut a 4.14.1 release @dlg99 ? |
merlimat
pushed a commit
that referenced
this pull request
May 29, 2021
### Motivation After add label for prometheus metric by #2650, it will cause prometheus metric format check failed when no label specified for a statsLogger. The metric list as follow. ``` replication_bookkeeper_client_bookkeeper_client_bookie_watcher_NEW_ENSEMBLE_TIME{success="false",quantile="0.9999", } NaN ``` ### Modification 1. add label empty check for `PrometheusTextFormatUtil` 2. add label scope check test cover 3. add prometheus metric regex pattern check in test case Reviewers: lipenghui <penghui@apache.org>, Andrey Yegorov <None>, Matteo Merli <mmerli@apache.org>, Jia Zhai <zhaijia@apache.org>, Addison Higham <None>, Enrico Olivelli <eolivelli@gmail.com> This closes #2718 from hangc0276/chenhang/fix_bookeeper_metric_bug and squashes the following commits: 8590704 [hangc0276] format code a6942d4 [chenhang] fix prometheus metric provider bug and add test to cover label scope and metric format check bb8b1e0 [Andrey Yegorov] Include gradle files into the source artifact for releases, exclude site2/** 732b6cf [Andrey Yegorov] [maven-release-plugin] prepare for next development iteration b0d9f10 [Andrey Yegorov] [maven-release-plugin] prepare branch branch-4.14 8dc108b [Matteo Merli] y 73e22ca [Don Inghram] ISSUE2620: RocksDB log path configurable 034ef85 [Shoothzj] Fix logger member not correct; (#2605) b824a60 [hangc0276] fix always select the same region set bug for RegionAwareEnsemblePlacementPolicy (#2658) 683ad45 [Matteo Merli] Allow to attach labels to metrics (#2650) 8091096 [Matteo Merli] Allow to bypass journal for writes (#2401) 63867a9 [Matteo Merli] Impose a memory limit on the bookie journal (#2710) 87579b0 [Matteo Merli] Read entry error should print lastAddConfirmed in the log (#2707)
Sunny-Island
pushed a commit
to Sunny-Island/bookkeeper
that referenced
this pull request
Jun 2, 2021
### Motivation After add label for prometheus metric by apache#2650, it will cause prometheus metric format check failed when no label specified for a statsLogger. The metric list as follow. ``` replication_bookkeeper_client_bookkeeper_client_bookie_watcher_NEW_ENSEMBLE_TIME{success="false",quantile="0.9999", } NaN ``` ### Modification 1. add label empty check for `PrometheusTextFormatUtil` 2. add label scope check test cover 3. add prometheus metric regex pattern check in test case Reviewers: lipenghui <penghui@apache.org>, Andrey Yegorov <None>, Matteo Merli <mmerli@apache.org>, Jia Zhai <zhaijia@apache.org>, Addison Higham <None>, Enrico Olivelli <eolivelli@gmail.com> This closes apache#2718 from hangc0276/chenhang/fix_bookeeper_metric_bug and squashes the following commits: 8590704 [hangc0276] format code a6942d4 [chenhang] fix prometheus metric provider bug and add test to cover label scope and metric format check bb8b1e0 [Andrey Yegorov] Include gradle files into the source artifact for releases, exclude site2/** 732b6cf [Andrey Yegorov] [maven-release-plugin] prepare for next development iteration b0d9f10 [Andrey Yegorov] [maven-release-plugin] prepare branch branch-4.14 8dc108b [Matteo Merli] y 73e22ca [Don Inghram] ISSUE2620: RocksDB log path configurable 034ef85 [Shoothzj] Fix logger member not correct; (apache#2605) b824a60 [hangc0276] fix always select the same region set bug for RegionAwareEnsemblePlacementPolicy (apache#2658) 683ad45 [Matteo Merli] Allow to attach labels to metrics (apache#2650) 8091096 [Matteo Merli] Allow to bypass journal for writes (apache#2401) 63867a9 [Matteo Merli] Impose a memory limit on the bookie journal (apache#2710) 87579b0 [Matteo Merli] Read entry error should print lastAddConfirmed in the log (apache#2707)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
After add label for prometheus metric by #2650, it will cause prometheus metric format check failed when no label specified for a statsLogger. The metric list as follow.
Modification
PrometheusTextFormatUtil