Skip to content

[improve][txn]PIP-160 make txn components supports buffered writer metrics - #17701

Merged
congbobo184 merged 1 commit into
apache:masterfrom
poorbarcode:pip/160-18
Sep 29, 2022
Merged

[improve][txn]PIP-160 make txn components supports buffered writer metrics#17701
congbobo184 merged 1 commit into
apache:masterfrom
poorbarcode:pip/160-18

Conversation

@poorbarcode

@poorbarcode poorbarcode commented Sep 17, 2022

Copy link
Copy Markdown
Contributor

Master Issue: #15370

Modifications


  • Delete constructor of TxnLogBufferedWriter without parameter metrics.
    • Motivation: it is unnecessary.

  • Add a default DisabledTxnLogBufferedWriterMetricsStats implementation.


Documentation

  • doc-required

  • doc-not-needed

  • doc

  • doc-complete

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 17, 2022
@poorbarcode
poorbarcode force-pushed the pip/160-18 branch 2 times, most recently from 2ca47e0 to 398a6e7 Compare September 17, 2022 08:41
@poorbarcode

Copy link
Copy Markdown
Contributor Author

This PR should to merge the following branches:

  • master

@poorbarcode poorbarcode reopened this Sep 20, 2022
@poorbarcode

Copy link
Copy Markdown
Contributor Author

/pulsarbot rerun-failure-checks

@liangyepianzhou liangyepianzhou left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work!

@congbobo184
congbobo184 merged commit 0f53c79 into apache:master Sep 29, 2022
@poorbarcode
poorbarcode deleted the pip/160-18 branch September 29, 2022 12:32
congbobo184 pushed a commit 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/transaction doc-not-needed Your PR changes do not impact docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants