grafana / beyla

eBPF-based autoinstrumentation of web applications and network metrics
https://grafana.com/oss/beyla-ebpf/
Apache License 2.0
1.23k stars 78 forks source link

Grafana: fix metric names for dashboard #678

Closed marevers closed 3 months ago

marevers commented 3 months ago

This PR fixes incorrect metric names in the Grafana dashboard.

Metrics names did not include _seconds_ e.g. http_server_request_duration_bucket should actually be http_server_request_duration_seconds_bucket and rpc_server_duration_bucket should be rpc_server_duration_seconds_bucket.

With these changes, the dashboard works out of the box. I assume the metric names were changed at some point, but the dashboard was not changed along with it accordingly.

CLAassistant commented 3 months ago

CLA assistant check
All committers have signed the CLA.

marctc commented 3 months ago

Thanks, nice catch. However, this might be on purpouse due conversion from OTEL to Prometheus metrics. is that right @mariomac ?

marevers commented 3 months ago

There were more (http_client and rpc_client) metrics with incorrect names. I fixed those too.

mariomac commented 3 months ago

Thank you for your contribution @marevers !

Actually, the dashboard was composed for an scenario where, between Beyla and Grafana, there was the Grafana Agent doing the OpenTelemetry ingestion and transforming the metrics to Mimir by using a given naming convention.

It seems that the main scenario is that our users send the data directly to the OpenTelemetry endpoint, which uses a different naming convention to transform the metrics, so we will internally discuss wether we should adopt the metrics that you are proposing and come back to this PR with a decision.

codecov-commenter commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 39.25%. Comparing base (490a82c) to head (70dbacb). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #678 +/- ## =========================================== - Coverage 77.40% 39.25% -38.16% =========================================== Files 96 93 -3 Lines 8016 7800 -216 =========================================== - Hits 6205 3062 -3143 - Misses 1470 4556 +3086 + Partials 341 182 -159 ``` | [Flag](https://app.codecov.io/gh/grafana/beyla/pull/678/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=grafana) | Coverage Δ | | |---|---|---| | [integration-test](https://app.codecov.io/gh/grafana/beyla/pull/678/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=grafana) | `?` | | | [unittests](https://app.codecov.io/gh/grafana/beyla/pull/678/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=grafana) | `39.25% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=grafana#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

marevers commented 3 months ago

It seems that the main scenario is that our users send the data directly to the OpenTelemetry endpoint, which uses a different naming convention to transform the metrics, so we will internally discuss wether we should adopt the metrics that you are proposing and come back to this PR with a decision.

@mariomac Thank you for responding. Upon a deeper look I also realized some label names are incorrect which is causing some sum by functions to not work. Should you accept this PR then I can fix that as well.

mariomac commented 3 months ago

After discussing internally, we will default the names as you propose in this PR. Thanks for your contribution @marevers !

marevers commented 3 months ago

@mariomac thank you. I will open a separate PR for the corrections in the label names.