Skip to content

[fix] [txn] Make txn metrics name conforms to the rule - #17905

Merged
congbobo184 merged 2 commits into
apache:masterfrom
poorbarcode:flaky/txn_metrics_name
Nov 16, 2022
Merged

[fix] [txn] Make txn metrics name conforms to the rule#17905
congbobo184 merged 2 commits into
apache:masterfrom
poorbarcode:flaky/txn_metrics_name

Conversation

@poorbarcode

@poorbarcode poorbarcode commented Sep 30, 2022

Copy link
Copy Markdown
Contributor

Fixes: #17921

Note:

This patch will change metrics names s_bufferedwriter_batch_record_count and s_bufferedwriter_batch_oldest_record_delay_time_second. These two names were first used in this PR #17701, and PR #17701 hasn't cherry-picked any branches yet, so this change will not cause any breaking changes.

Motivation

https://github.com/poorbarcode/pulsar/actions/runs/3156649582/jobs/5136584463
https://github.com/apache/pulsar/actions/runs/3156649597/jobs/5136596447

Problem-1

If the Prometheus-Colloctor which typed Counter is named 'xxx_count', then the output metrics-api will be named 'xxx_count_count'.

TxnLogBufferedWriterMetricsStats hits this error.

String recordsPerBatchMetricName =
String.format("%s_bufferedwriter_batch_record_count", metricsPrefix);


Problem-2

PrometheusMetricsTest defines the standard metrics name(see code below):

["_sum", "_bucket", "_count", "_total", "_created"]

But the standard Prometheus name has three others( see: https://github.com/prometheus/client_java/blob/c28b901225e35e7c1df0eacae8b58fdfbb390162/simpleclient/src/main/java/io/prometheus/client/Collector.java#L152-L186 ):

["_info", "_gsum", "_gcount"]

if (metricName.endsWith("_sum")) {
String summaryMetricName = metricName.substring(0, metricName.indexOf("_sum"));
if (!typeDefs.containsKey(summaryMetricName)) {
fail("Metric " + metricName + " does not have a corresponding summary type definition");
}
} else if (metricName.endsWith("_count")) {
String summaryMetricName = metricName.substring(0, metricName.indexOf("_count"));
if (!typeDefs.containsKey(summaryMetricName)) {
fail("Metric " + metricName + " does not have a corresponding summary type definition");
}
} else if (metricName.endsWith("_bucket")) {
String summaryMetricName = metricName.substring(0, metricName.indexOf("_bucket"));
if (!typeDefs.containsKey(summaryMetricName)) {
fail("Metric " + metricName + " does not have a corresponding summary type definition");
}
} else if (metricName.endsWith("_created")) {
String summaryMetricName = metricName.substring(0, metricName.indexOf("_created"));
if (!typeDefs.containsKey(summaryMetricName)) {
fail("Metric " + metricName + " does not have a corresponding summary type definition");
}
} else if (metricName.endsWith("_total")) {
String summaryMetricName = metricName.substring(0, metricName.indexOf("_total"));
if (!typeDefs.containsKey(summaryMetricName)) {
fail("Metric " + metricName + " does not have a corresponding counter type definition");
}
} else {
fail("Metric " + metricName + " does not have a type definition");
}


Modifications

  • Make PrometheusMetricsTest run with transaction feature
  • Make txn metrics name conforms to the rule. see: https://prometheus.io/docs/practices/naming/
  • Make PrometheusMetricsTest support all suffix of prometheus metrics name

Documentation

  • doc-not-needed
    (Please explain why)

Matching PR in forked repository

PR in forked repository:

@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Sep 30, 2022
@poorbarcode
poorbarcode force-pushed the flaky/txn_metrics_name branch 2 times, most recently from 696e30c to f50a463 Compare September 30, 2022 09:06
@codelipenghui

Copy link
Copy Markdown
Contributor

@poorbarcode Please help confirm that we don't have a release for the metrics name that you want change.

@poorbarcode

Copy link
Copy Markdown
Contributor Author

Hi @codelipenghui

Yes, we don't have a release for these metrics names that I want to change. Thanks

@poorbarcode
poorbarcode force-pushed the flaky/txn_metrics_name branch from f50a463 to 55dc2c0 Compare November 10, 2022 13:37
@codelipenghui

Copy link
Copy Markdown
Contributor

These two names were first used in this PR #17701, and will be released in version 2.11.1

We will not release 17701 in 2.11.1 right?

@codelipenghui

Copy link
Copy Markdown
Contributor

/pulsarbot run-failure-checks

@poorbarcode

Copy link
Copy Markdown
Contributor Author

@codelipenghui

We will not release 17701 in 2.11.1 right?

Correct, PR #17701 hasn't cherry-picked any branches yet

@poorbarcode poorbarcode changed the title [fix][txn]Make txn metrics name conforms to the rule [fix] [txn] Make txn metrics name conforms to the rule Nov 11, 2022
@codecov-commenter

codecov-commenter commented Nov 11, 2022

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 47.20%. Comparing base (79a97a9) to head (55dc2c0).
⚠️ Report is 3352 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #17905      +/-   ##
============================================
- Coverage     47.24%   47.20%   -0.05%     
+ Complexity    10407    10405       -2     
============================================
  Files           692      692              
  Lines         67766    67766              
  Branches       7258     7258              
============================================
- Hits          32016    31988      -28     
- Misses        32158    32193      +35     
+ Partials       3592     3585       -7     
Flag Coverage Δ
unittests 47.20% <ø> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 37 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.

@congbobo184
congbobo184 merged commit 3715934 into apache:master Nov 16, 2022
pgier added a commit to pgier/pulsar that referenced this pull request Nov 17, 2022
…e#17905)"

This reverts commit 3715934.
This change broke several unit tests around metrics.  It should be reverted until the
associated tests are fixed.

Signed-off-by: Paul Gier <paul.gier@datastax.com>
pgier added a commit to pgier/pulsar that referenced this pull request Nov 17, 2022
…e#17905)

This reverts commit 3715934.
This change broke several unit tests around metrics.  It should be reverted until the
associated tests are fixed.

Signed-off-by: Paul Gier <paul.gier@datastax.com>
@poorbarcode
poorbarcode deleted the flaky/txn_metrics_name branch November 21, 2022 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky-test: PrometheusMetricsTest.testDuplicateMetricTypeDefinitions

4 participants