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(static-file): replace string metric name with enum #8782

Closed
wants to merge 3 commits into from

Conversation

9547
Copy link
Contributor

@9547 9547 commented Jun 12, 2024

close #8758

FYI, this PR also changed the reth_static_files_xxx metric names(from label to name):

-reth_static_files_jar_provider_calls_total{segment="headers",operation="prune"}  0
+reth_static_files_jar_provider_headers_prune_total 0

BTW, I'm not quite satisfied with the 18 match arms(match (segment, operation)), don't know if there are any other solutions.

9547 added 3 commits June 13, 2024 02:06
Signed-off-by: 9547 <29431502+9547@users.noreply.github.com>
Signed-off-by: 9547 <29431502+9547@users.noreply.github.com>
Signed-off-by: 9547 <29431502+9547@users.noreply.github.com>
@Rjected
Copy link
Member

Rjected commented Jun 12, 2024

BTW, I'm not quite satisfied with the 18 match arms(match (segment, operation)), don't know if there are any other solutions.

Might be a good idea to have separate metrics structs for each segment, and separate methods for each segment which handle the recording for each provider operation.

@mattsse mattsse added the A-observability Related to tracing, metrics, logs and other observability tools label Jun 12, 2024
@kamuik16
Copy link
Contributor

Hey! This will be conflicting with #8718. should prob merge that one first?

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

ah okay, this basically flattens the map and adds dedicated metrics for all variants

I this this is better because now it's visible what metrics we have, the match isn't that bad

ptal @joshieDo @shekhirin

@Rjected
Copy link
Member

Rjected commented Jun 13, 2024

After some discussion with @shekhirin we actually decided to go with a different approach than the initial issue entirely. The underlying problem with the use of labels right now is that it somewhat obscures how to go from a metric name in grafana, to where the metric is actually set in the codebase.

With MakeCanonicalMetrics, the issue was that the graphs were poorly implemented and would need to be revised as well, whereas here that is not the case.

I think the best approach would be to add descriptions to each metric that uses labels, indicating where in the code to find that metric, rather than removing labels from the codebase.

Labels for this type of thing are best practices w.r.t prometheus naming:
https://prometheus.io/docs/practices/naming/

Thank you for your effort though @9547!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-observability Related to tracing, metrics, logs and other observability tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace as_str-based static files duration metrics with enums
4 participants