Skip to content

fix failed to generate prometheus stats for transaction#10899

Closed
xuesongxs wants to merge 5 commits intoapache:masterfrom
xuesongxs:transactions-metrics
Closed

fix failed to generate prometheus stats for transaction#10899
xuesongxs wants to merge 5 commits intoapache:masterfrom
xuesongxs:transactions-metrics

Conversation

@xuesongxs
Copy link
Contributor

fix #10898

modify regular expression.

@eolivelli
Copy link
Contributor

thank you @xuesongxs
can you please add a unit test that demonstrate the problem and ensures that we won't see regressions in the future ?

@xuesongxs
Copy link
Contributor Author

thank you @xuesongxs
can you please add a unit test that demonstrate the problem and ensures that we won't see regressions in the future ?

This feature has been included in ManagedLedgerMetricsTest.testManagedLedgerMetrics().

@eolivelli
Copy link
Contributor

This feature has been included in ManagedLedgerMetricsTest.testManagedLedgerMetrics().
Sorry, I do not understand your answer.

If you change some line in the code, this should be an effect on tests:

  • make them fail , and in this case you usually have to modify the tests
  • in case the code you touch is not covered by a test, then you have to add a test

it looks strange to me that you are changing a constant and there is no change in the tests.

@xuesongxs
Copy link
Contributor Author

@eolivelli I've added unit test case.

@sijie
Copy link
Member

sijie commented Jun 18, 2021

@gaoran10 @congbobo184 @codelipenghui Can you review this PR?

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

@eolivelli eolivelli assigned xuesongxs and unassigned eolivelli Jun 18, 2021
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.

@xuesongxs in 2.8.0 transaction will not generate wrong managedLedger name, so we shouldn't need to fix this problem.

@codelipenghui
Copy link
Contributor

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

@github-actions
Copy link

github-actions bot commented Mar 4, 2022

@xuesongxs: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)

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Failed to generate prometheus stats for transaction

5 participants

Comments