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

[ci-run][fix][txn]Make txn metrics name conforms to the rule #19

Closed
wants to merge 2 commits into from

Conversation

poorbarcode
Copy link
Owner

@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Oct 31, 2022
@poorbarcode poorbarcode force-pushed the flaky/txn_metrics_name branch from f50a463 to 55dc2c0 Compare November 10, 2022 13:37
congbobo184 pushed a commit to apache/pulsar that referenced this pull request Nov 16, 2022
Fixes: #17921

<strong>Note</strong>: 

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.

https://github.com/apache/pulsar/blob/fb7307d8f4998e42b18df3a4599fd7ec34cb04a9/pulsar-transaction/coordinator/src/main/java/org/apache/pulsar/transaction/coordinator/impl/TxnLogBufferedWriterMetricsStats.java#L105-L106


----

#### 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"]
```


https://github.com/apache/pulsar/blob/fb7307d8f4998e42b18df3a4599fd7ec34cb04a9/pulsar-broker/src/test/java/org/apache/pulsar/broker/stats/PrometheusMetricsTest.java#L834-L861

----

### 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

- [x] `doc-not-needed` 
(Please explain why)

### Matching PR in forked repository

PR in forked repository:

- poorbarcode#19
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant