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

Rename Pulsar txn metrics to specify OpenMetrics #16581

Merged
merged 1 commit into from
Jul 15, 2022

Conversation

hezhangjian
Copy link
Member

@hezhangjian hezhangjian commented Jul 13, 2022

Motivation

See https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md
A COUNTER needs metrics_name_total or metrics_name_created

This PR contains metric name broken changes.

Modifications

Rename counter's _count to _total

metrics name broken changes

  • rename pulsar_txn_committed_count to pulsar_txn_committed_total
  • rename pulsar_txn_aborted_count to pulsar_txn_aborted_total
  • rename pulsar_txn_created_count to pulsar_txn_created_total
  • rename pulsar_txn_timeout_count to pulsar_txn_timeout_total
  • rename pulsar_txn_append_log_count to pulsar_txn_append_log_total

Documentation

  • doc
    As mentioned above, the metrics name has changed

@hezhangjian hezhangjian self-assigned this Jul 13, 2022
@codelipenghui codelipenghui added this to the 2.11.0 milestone Jul 13, 2022
@codelipenghui codelipenghui added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/metrics release/important-notice The changes which are important should be mentioned in the release note labels Jul 13, 2022
@github-actions
Copy link

@Shoothzj Please provide a correct documentation label for your PR.
Instructions see Pulsar Documentation Label Guide.

Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

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

This is a breaking change. If we really want to go with this path we should keep both the versions, deprecate the older ones and eventually remove them in a future version

Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

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

LGTM.

@nicoloboschi - you are correct that this is a breaking change. @Shoothzj proposed this change on the mailing list: https://lists.apache.org/thread/ojwn2l2rtp4lyyrjbhmnf0m6cprjfwx8. The thread mentions that upgrading the prometheus client depends up fixing counters that do not end with _total. We have already merged #13785. I don't think we can easily make it work to expose both metrics, so as long as we communicate to our users, I think this is okay.

@hezhangjian hezhangjian added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. and removed doc-label-missing labels Jul 14, 2022
Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

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

Thanks @michaeljmarshall for pointing the ML discussion.
I agree that sooner or later we have to move to newer versions of Prometheus and it's better to rename all the needed metrics in the same release

@hezhangjian
Copy link
Member Author

@nicoloboschi I will follow up on this. Ensure all the metrics to be renamed before 2.11.0

@hezhangjian
Copy link
Member Author

/pulsarbot run-failure-checks

@hezhangjian hezhangjian merged commit f3dbd3a into apache:master Jul 15, 2022
@hezhangjian hezhangjian deleted the rename-txn-metrics branch July 15, 2022 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/metrics doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. release/important-notice The changes which are important should be mentioned in the release note type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants