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

feat: metrics for request number per route #105

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

Conversation

Garrethta
Copy link

No description provided.

@Garrethta Garrethta requested a review from SkeLLLa as a code owner July 13, 2024 11:06
@SkeLLLa
Copy link
Owner

SkeLLLa commented Jul 22, 2024

Hi. Can you elaborate a bit why do you need a dedicated counter? I'm asking since histogram and summary metrics already expose counter as well. So you can reuse it.

@Garrethta
Copy link
Author

Garrethta commented Jul 22, 2024

Hi. Our API consists of routes belonging to different parts of the system that sometimes experience abnormal request amount spikes. A summary metric does not give us information about which route, or group of routes, is under pressure rn and which part of system needs scaling. This pr is an impl of counter metric from this lib but with prom interface.

@SkeLLLa
Copy link
Owner

SkeLLLa commented Aug 7, 2024

@Garrethta currently the following metrics are exposed:

          'http_request_duration_seconds_count{method="GET",route="/test",status_code="200"} 1',
          'http_request_duration_seconds_count{method="POST",route="/test",status_code="200"} 1',
          'http_request_summary_seconds_count{method="GET",route="/test",status_code="200"} 1',
          'http_request_summary_seconds_count{method="POST",route="/test",status_code="200"} 1',

Your PR adding a new metric that looks like:

          'route_request_counter{route="/test",status_code="200"} 2'

So my point is why not to use prometheus function like sum (http_request_summary_seconds_count) by (route, status_code) which will give you exactly the same numbers as added metric.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants