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.

Sign up for GitHub

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

Jump to bottom

[monitor][txn] Add metrics for transaction #15140

Merged
merged 25 commits into from
Aug 12, 2022
Merged

[monitor][txn] Add metrics for transaction #15140

merged 25 commits into from
Aug 12, 2022

Conversation

tjiuming
Copy link
Contributor

@tjiuming tjiuming commented Apr 12, 2022 •
edited by github-actions bot
Loading

Motivation

add metrics for transaction

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

Check the box below or label this PR directly (if you have committer privilege).

Need to update docs?

  • doc-required

    (If you need help on updating docs, create a doc issue)

  • no-need-doc

    (Please explain why)

  • doc

    (If this PR contains doc changes)

  • doc-complete

ariestp316@gmail.com added 5 commits April 12, 2022 16:39
tx metrics
4501550
transaction buffer client metrics
0303a84
Merge branch 'master' of https://github.com/tjiuming/pulsar into dev/…
881f3b0 
…tx_metrics
Merge branch 'master' of https://github.com/tjiuming/pulsar into dev/…
f0a7c61 
…tx_metrics

� Conflicts:
�	pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TransactionBufferClientImpl.java
�	pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/buffer/TransactionBufferClientTest.java
merge master
bb75481
@tjiuming tjiuming marked this pull request as ready for review April 12, 2022 17:27
@tjiuming
Copy link
Contributor Author

Tests to be completed

@github-actions
Copy link

@tjiuming:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@github-actions
Copy link

@tjiuming:Thanks for providing doc info!

@github-actions github-actions bot added doc-required Your PR changes impact docs and you will update later. and removed doc-label-missing labels Apr 12, 2022
@Anonymitaet
Copy link
Member

@momo-jun a soft reminder: here is a PR w/ doc-required label, could u pls follow up? Thanks

congbobo184
congbobo184 reviewed Apr 13, 2022
View reviewed changes
Copy link
Contributor

@congbobo184 congbobo184 left a comment

Choose a reason for hiding this comment

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

Good work! as the seem as tb, we should add tp stats

pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/prometheus/TopicStats.java Show resolved Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/prometheus/TopicStats.java Outdated Show resolved Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/prometheus/TopicStats.java Outdated Show resolved Hide resolved
@tjiuming
Copy link
Contributor Author

Good work! as the seem as tb, we should add tp stats

I know TC means TransactionCoordinator and TB means TransactionBuffer, but what means TP?

@tjiuming
Copy link
Contributor Author

Good work! as the seem as tb, we should add tp stats

I know TC means TransactionCoordinator and TB means TransactionBuffer, but what means TP?

Does it mean TransactionPendingAckStore ?

1 similar comment
@tjiuming
Copy link
Contributor Author

Good work! as the seem as tb, we should add tp stats

I know TC means TransactionCoordinator and TB means TransactionBuffer, but what means TP?

Does it mean TransactionPendingAckStore ?

@momo-jun
Copy link
Contributor

Hi @tjiuming will you submit a doc PR as soon as this PR is approved and merged? Ping me if you need any review or other assistance.

@tjiuming
Copy link
Contributor Author

I‘ll create another PR to add doc after this PR finished

@congbobo184
Copy link
Contributor

congbobo184 commented Apr 15, 2022 •
edited
Loading

@tjiuming yes PendingAck is TP

@congbobo184 congbobo184 changed the title [transaction][monitor] Add metrics for transaction [monitor][txn] Add metrics for transaction Apr 18, 2022
@tjiuming
Copy link
Contributor Author

@codelipenghui @gaoran10 Could you please help review?

@tjiuming tjiuming mentioned this pull request Apr 19, 2022
Merged
4 tasks
@congbobo184 congbobo184 added area/metrics doc-required Your PR changes impact docs and you will update later. and removed doc-complete Your PR changes impact docs and the related docs have been already added. labels Jul 4, 2022
@dao-jun
Merge branch 'master' of https://github.com/tjiuming/pulsar into dev/…
b252040 
…tx_metrics

� Conflicts:
�	pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/pendingack/impl/PendingAckHandleImpl.java
�	pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTest.java
eolivelli
eolivelli approved these changes Jul 20, 2022
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

shoothzj
shoothzj requested changes Jul 20, 2022
View reviewed changes
Copy link
Member

@shoothzj shoothzj left a comment

Choose a reason for hiding this comment

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

new metrics should ends with "_total"

See https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md

Also see

@tjiuming
Copy link
Contributor Author

tjiuming commented Jul 20, 2022 •
edited
Loading

new metrics should ends with "_total"

See https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md

Also see

it makes sense, I'll add _total suffix for non-prometheus managed metrics.

@tjiuming
Copy link
Contributor Author

@shoothzj PTAL

@tjiuming tjiuming requested a review from shoothzj July 20, 2022 08:55
@codelipenghui codelipenghui modified the milestones: 2.11.0, 2.12.0 Jul 26, 2022
@dao-jun
Merge branch 'master' into dev/tx_metrics
cee5625
@dao-jun
fix txn test
ee46bbb
@dao-jun
Merge branch 'master' into dev/tx_metrics
471dc37 
# Conflicts:
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/stats/prometheus/NamespaceStatsAggregator.java
#	pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/stats/TopicStatsImpl.java
@tjiuming
Copy link
Contributor Author

tjiuming commented Aug 9, 2022

/pulsarbot run-failure-checks

@codelipenghui codelipenghui merged commit b7b2e37 into apache:master Aug 12, 2022
coderzc pushed a commit to coderzc/pulsar that referenced this pull request Aug 13, 2022
@tjiuming @coderzc
[monitor][txn] Add metrics for transaction (apache#15140)
cc9ad72
Technoboy- pushed a commit to Technoboy-/pulsar that referenced this pull request Aug 14, 2022
@tjiuming @Technoboy-
[monitor][txn] Add metrics for transaction (apache#15140)
9030b06
@tjiuming tjiuming deleted the dev/tx_metrics branch August 14, 2022 12:51
Technoboy- pushed a commit to merlimat/pulsar that referenced this pull request Aug 16, 2022
@tjiuming @Technoboy-
[monitor][txn] Add metrics for transaction (apache#15140)
3480ba5
@momo-jun momo-jun added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-required Your PR changes impact docs and you will update later. labels Sep 6, 2022
Technoboy- pushed a commit that referenced this pull request Feb 8, 2023
@tjiuming @Technoboy-
[monitor][txn] Add metrics for transaction (#15140)
afc7f2f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@congbobo184 congbobo184 congbobo184 approved these changes

@nicoloboschi nicoloboschi nicoloboschi left review comments

@gaoran10 gaoran10 gaoran10 approved these changes

@shoothzj shoothzj shoothzj approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@eolivelli eolivelli eolivelli approved these changes

Assignees

@tjiuming tjiuming

Labels
area/metrics area/transaction cherry-picked/branch-2.11 doc-complete Your PR changes impact docs and the related docs have been already added. lifecycle/stale release/2.11.1
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.