[fix][broker] Correctly set byte and message out totals per topic. - #2
Closed
pgier wants to merge 1 commit into
Closed
[fix][broker] Correctly set byte and message out totals per topic.#2pgier wants to merge 1 commit into
pgier wants to merge 1 commit into
Conversation
14 tasks
pgier
force-pushed
the
fix-pulsar-messages-out-metric-reset
branch
6 times, most recently
from
November 17, 2022 16:15
216ec00 to
b88a386
Compare
Fixes apache#15819 The existing code calculates the pulsar_out_bytes_total and pulsar_out_messages_total per subscription metrics by adding the values from the currently connected consumers. This produces incorrect values as soon as one or more of the consumers disconnects from the subscription. This updates these two metrics to directly use the subscription stats for these values, and match the output of `pulsar-admin topic stats`. It also changes these metrics to be defined as `counter` type. Signed-off-by: Paul Gier <paul.gier@datastax.com>
pgier
force-pushed
the
fix-pulsar-messages-out-metric-reset
branch
from
November 17, 2022 20:07
b88a386 to
62514d5
Compare
michaeljmarshall
pushed a commit
to apache/pulsar
that referenced
this pull request
Nov 19, 2022
…ion (#18451) Signed-off-by: Paul Gier <paul.gier@datastax.com> Fixes #15819 The existing code calculates the pulsar_out_bytes_total and pulsar_out_messages_total per subscription metrics by adding the values from the currently connected consumers. This produces incorrect values as soon as one or more of the consumers disconnects from the subscription. This changes these two metrics to directly use the subscription stats for these values, and match the output of `pulsar-admin topic stats`. Signed-off-by: Paul Gier <paul.gier@datastax.com> Fixes #15819 ### Motivation The prometheus metrics for pulsar_out_bytes_total and pulsar_out_messages_total should never decrease, and they should match the output seen when using pulsar-admin. ### Modifications Changed the calculation of pulsar_out_bytes_total and pulsar_out_messages_total to directly use the subscription stats instead of calculating these values by summing the values of the currently connected consumers. ### Verifying this change - [X] Make sure that the change passes the CI checks. Added a unit test to cover this case. ### Does this pull request potentially affect one of the following parts: *If the box was checked, please highlight the changes* - [ ] Dependencies (add or upgrade a dependency) - [ ] The public API - [ ] The schema - [ ] The default values of configurations - [ ] The threading model - [ ] The binary protocol - [ ] The REST endpoints - [ ] The admin CLI options - [ ] Anything that affects deployment ### Documentation <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. --> - [ ] `doc` <!-- Your PR contains doc changes. Please attach the local preview screenshots (run `sh start.sh` at `pulsar/site2/website`) to your PR description, or else your PR might not get merged. --> - [ ] `doc-required` <!-- Your PR changes impact docs and you will update later --> - [X] `doc-not-needed` <!-- Your PR changes do not impact docs --> - [ ] `doc-complete` <!-- Docs have been already added --> ### Matching PR in forked repository PR in forked repository: pgier#2
michaeljmarshall
pushed a commit
to apache/pulsar
that referenced
this pull request
Nov 19, 2022
…ion (#18451) Signed-off-by: Paul Gier <paul.gier@datastax.com> Fixes #15819 The existing code calculates the pulsar_out_bytes_total and pulsar_out_messages_total per subscription metrics by adding the values from the currently connected consumers. This produces incorrect values as soon as one or more of the consumers disconnects from the subscription. This changes these two metrics to directly use the subscription stats for these values, and match the output of `pulsar-admin topic stats`. Signed-off-by: Paul Gier <paul.gier@datastax.com> Fixes #15819 ### Motivation The prometheus metrics for pulsar_out_bytes_total and pulsar_out_messages_total should never decrease, and they should match the output seen when using pulsar-admin. ### Modifications Changed the calculation of pulsar_out_bytes_total and pulsar_out_messages_total to directly use the subscription stats instead of calculating these values by summing the values of the currently connected consumers. ### Verifying this change - [X] Make sure that the change passes the CI checks. Added a unit test to cover this case. ### Does this pull request potentially affect one of the following parts: *If the box was checked, please highlight the changes* - [ ] Dependencies (add or upgrade a dependency) - [ ] The public API - [ ] The schema - [ ] The default values of configurations - [ ] The threading model - [ ] The binary protocol - [ ] The REST endpoints - [ ] The admin CLI options - [ ] Anything that affects deployment ### Documentation <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. --> - [ ] `doc` <!-- Your PR contains doc changes. Please attach the local preview screenshots (run `sh start.sh` at `pulsar/site2/website`) to your PR description, or else your PR might not get merged. --> - [ ] `doc-required` <!-- Your PR changes impact docs and you will update later --> - [X] `doc-not-needed` <!-- Your PR changes do not impact docs --> - [ ] `doc-complete` <!-- Docs have been already added --> ### Matching PR in forked repository PR in forked repository: pgier#2 (cherry picked from commit c03e33e)
michaeljmarshall
pushed a commit
to apache/pulsar
that referenced
this pull request
Nov 19, 2022
…ion (#18451) Signed-off-by: Paul Gier <paul.gier@datastax.com> Fixes #15819 The existing code calculates the pulsar_out_bytes_total and pulsar_out_messages_total per subscription metrics by adding the values from the currently connected consumers. This produces incorrect values as soon as one or more of the consumers disconnects from the subscription. This changes these two metrics to directly use the subscription stats for these values, and match the output of `pulsar-admin topic stats`. Signed-off-by: Paul Gier <paul.gier@datastax.com> Fixes #15819 ### Motivation The prometheus metrics for pulsar_out_bytes_total and pulsar_out_messages_total should never decrease, and they should match the output seen when using pulsar-admin. ### Modifications Changed the calculation of pulsar_out_bytes_total and pulsar_out_messages_total to directly use the subscription stats instead of calculating these values by summing the values of the currently connected consumers. ### Verifying this change - [X] Make sure that the change passes the CI checks. Added a unit test to cover this case. ### Does this pull request potentially affect one of the following parts: *If the box was checked, please highlight the changes* - [ ] Dependencies (add or upgrade a dependency) - [ ] The public API - [ ] The schema - [ ] The default values of configurations - [ ] The threading model - [ ] The binary protocol - [ ] The REST endpoints - [ ] The admin CLI options - [ ] Anything that affects deployment ### Documentation <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. --> - [ ] `doc` <!-- Your PR contains doc changes. Please attach the local preview screenshots (run `sh start.sh` at `pulsar/site2/website`) to your PR description, or else your PR might not get merged. --> - [ ] `doc-required` <!-- Your PR changes impact docs and you will update later --> - [X] `doc-not-needed` <!-- Your PR changes do not impact docs --> - [ ] `doc-complete` <!-- Docs have been already added --> ### Matching PR in forked repository PR in forked repository: pgier#2 (cherry picked from commit c03e33e)
Owner
Author
|
Closing this because it's been merged into the apache repo |
nicoloboschi
pushed a commit
to datastax/pulsar
that referenced
this pull request
Nov 23, 2022
…ion (apache#18451) Signed-off-by: Paul Gier <paul.gier@datastax.com> Fixes apache#15819 The existing code calculates the pulsar_out_bytes_total and pulsar_out_messages_total per subscription metrics by adding the values from the currently connected consumers. This produces incorrect values as soon as one or more of the consumers disconnects from the subscription. This changes these two metrics to directly use the subscription stats for these values, and match the output of `pulsar-admin topic stats`. Signed-off-by: Paul Gier <paul.gier@datastax.com> Fixes apache#15819 ### Motivation The prometheus metrics for pulsar_out_bytes_total and pulsar_out_messages_total should never decrease, and they should match the output seen when using pulsar-admin. ### Modifications Changed the calculation of pulsar_out_bytes_total and pulsar_out_messages_total to directly use the subscription stats instead of calculating these values by summing the values of the currently connected consumers. ### Verifying this change - [X] Make sure that the change passes the CI checks. Added a unit test to cover this case. ### Does this pull request potentially affect one of the following parts: *If the box was checked, please highlight the changes* - [ ] Dependencies (add or upgrade a dependency) - [ ] The public API - [ ] The schema - [ ] The default values of configurations - [ ] The threading model - [ ] The binary protocol - [ ] The REST endpoints - [ ] The admin CLI options - [ ] Anything that affects deployment ### Documentation <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. --> - [ ] `doc` <!-- Your PR contains doc changes. Please attach the local preview screenshots (run `sh start.sh` at `pulsar/site2/website`) to your PR description, or else your PR might not get merged. --> - [ ] `doc-required` <!-- Your PR changes impact docs and you will update later --> - [X] `doc-not-needed` <!-- Your PR changes do not impact docs --> - [ ] `doc-complete` <!-- Docs have been already added --> ### Matching PR in forked repository PR in forked repository: pgier#2 (cherry picked from commit c03e33e) (cherry picked from commit 54dccf9)
lifepuzzlefun
pushed a commit
to lifepuzzlefun/pulsar
that referenced
this pull request
Dec 9, 2022
…ion (apache#18451) Signed-off-by: Paul Gier <paul.gier@datastax.com> Fixes apache#15819 The existing code calculates the pulsar_out_bytes_total and pulsar_out_messages_total per subscription metrics by adding the values from the currently connected consumers. This produces incorrect values as soon as one or more of the consumers disconnects from the subscription. This changes these two metrics to directly use the subscription stats for these values, and match the output of `pulsar-admin topic stats`. Signed-off-by: Paul Gier <paul.gier@datastax.com> Fixes apache#15819 ### Motivation The prometheus metrics for pulsar_out_bytes_total and pulsar_out_messages_total should never decrease, and they should match the output seen when using pulsar-admin. ### Modifications Changed the calculation of pulsar_out_bytes_total and pulsar_out_messages_total to directly use the subscription stats instead of calculating these values by summing the values of the currently connected consumers. ### Verifying this change - [X] Make sure that the change passes the CI checks. Added a unit test to cover this case. ### Does this pull request potentially affect one of the following parts: *If the box was checked, please highlight the changes* - [ ] Dependencies (add or upgrade a dependency) - [ ] The public API - [ ] The schema - [ ] The default values of configurations - [ ] The threading model - [ ] The binary protocol - [ ] The REST endpoints - [ ] The admin CLI options - [ ] Anything that affects deployment ### Documentation <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. --> - [ ] `doc` <!-- Your PR contains doc changes. Please attach the local preview screenshots (run `sh start.sh` at `pulsar/site2/website`) to your PR description, or else your PR might not get merged. --> - [ ] `doc-required` <!-- Your PR changes impact docs and you will update later --> - [X] `doc-not-needed` <!-- Your PR changes do not impact docs --> - [ ] `doc-complete` <!-- Docs have been already added --> ### Matching PR in forked repository PR in forked repository: pgier#2
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes apache#15819
The existing code calculates the pulsar_out_bytes_total and pulsar_out_messages_total per subscription metrics by adding the values from the currently connected consumers. This produces incorrect values as soon as one or more of the consumers disconnects from the subscription.
This changes these two metrics to directly use the subscription stats for these values, and match the output of
pulsar-admin topic stats.Signed-off-by: Paul Gier paul.gier@datastax.com
Fixes apache#15819
Motivation
The prometheus metrics for pulsar_out_bytes_total and pulsar_out_messages_total should never decrease,
and they should match the output seen when using pulsar-admin.
Modifications
Changed the calculation of pulsar_out_bytes_total and pulsar_out_messages_total to directly use the
subscription stats instead of calculating these values by summing the values of the currently connected
consumers.
Verifying this change
Added a unit test to cover this case.
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: N/A