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

Fix/metric emission tracking #2825 #2870

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fix/metric emission tracking #2825 #2870

wants to merge 5 commits into from

Conversation

snehavats1404
Copy link

@snehavats1404 snehavats1404 commented Jan 13, 2025 •
edited
Loading

What problem does this PR solve?

Issue Number: #2825

Problem Summary:
The issue pertains to incorrectly tracking and emitting metric names when dumping Prometheus metrics. Some metrics, especially those related to latency, were not being properly emitted due to incorrect handling of the emitted_metrics set.

What is changed and the side effects?

Changed:

  • Fixed the logic for tracking emitted metrics by using a set to ensure that duplicate metrics are not emitted.
  • Ensured that all necessary latency percentiles and summary metrics are properly output, with correct handling of the _latency_* and _count suffixes.
  • Added a mechanism to track previously emitted metric names to prevent duplication in Prometheus output.

Side effects:

  • Performance effects: The introduction of a set to track emitted metrics could slightly impact performance, but this change ensures more efficient handling of duplicate metrics.
  • Breaking backward compatibility: No breaking changes; existing behavior should remain intact except for the fix to prevent duplicate metric emission.

@snehavats1404 snehavats1404 changed the title Fix/metric emission tracking #2885 Fix/metric emission tracking #2825 Jan 13, 2025
chenBright
chenBright reviewed Jan 13, 2025
View reviewed changes
src/brpc/builtin/prometheus_metrics_service.cpp Outdated
Comment on lines 106 to 110
if(emitted_metrics.find(metrics_name.as_string()) == emitted_metrics.end()){
*_os<< "# HELP " << metrics_name <<'\n'
<< "# TYPE " << metrics_name <<" gauge\n";
emitted_metrics.insert(metrics_name.as_string());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Some spaces are missing.

@chenBright
Copy link
Contributor

chenBright commented Jan 13, 2025 •
edited
Loading

  1. If the bvar of mbvar is not continuously input into the dump, is there a problem?
  2. In addition, it is necessary to add unit test cases.

@chenBright chenBright linked an issue Jan 13, 2025 that may be closed by this pull request
brpc的brpc_metrics的输出格式与prometheus的标准不符，尤其MultiDimension<bvar::LatencyRecorder>方式 #2825
Open
@snehavats1404
Copy link
Author

I don't feel there must be any issue in not adding any unit testcase, if there is any please let me know.

@chenBright
Copy link
Contributor

I don't feel there must be any issue in not adding any unit testcase, if there is any please let me know.

We hope that PR will be equipped with unit tests as much as possible to ensure correctness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chenBright chenBright chenBright left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@snehavats1404 @chenBright