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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[馃挕 FEATURE REQUEST]: Add default labels for rr metrics #1632

Open
asanikovich opened this issue Jun 28, 2023 · 12 comments
Open

[馃挕 FEATURE REQUEST]: Add default labels for rr metrics #1632

asanikovich opened this issue Jun 28, 2023 · 12 comments
Assignees
Labels
C-feature-request Category: feature requested, but need to be discussed help-needed-easy Call for participation: Experience needed to fix: Easy / not much

Comments

@asanikovich
Copy link

asanikovich commented Jun 28, 2023

Plugin

Metrics

I have an idea!

I have an idea, listen to me!!
Currently difficultly to distinguish metrics with many rr pods. (e.x. in K8s).
Add to config parameter 'metrics' list of labels:

metrics:
  labels:
    - env: "APP_ENV"
    - app: "APP_NAME"

Format - label_name: ENV_NAME (value will be read from ENV variable)

Which should be added to all default metrics by all plugins described at https://roadrunner.dev/docs/lab-metrics/current/en
For custom application metrics - labels should be defined by user (as is).

@asanikovich asanikovich added the C-feature-request Category: feature requested, but need to be discussed label Jun 28, 2023
@rustatian
Copy link
Member

Hey @asanikovich 馃憢馃徎
You can add a pod name (or IP address) to the specific metric in the dashboard. Our dashboards are examples of all possible RR metrics.

Custom metrics you are able to declare via configuration or directly from the PHP worker.

@asanikovich
Copy link
Author

@rustatian
The best practice is not to define some extra parameters in Prometheus.
Each metric should contain all the needed information.

In the case of running RR in k8s in many pods, we have next metrics:

{__name__="rr_http_uptime_seconds", instance="main-1.svc.cluster.local:8088", job="dev-main"}
{__name__="rr_http_uptime_seconds", instance="main-2.svc.cluster.local:8088", job="dev-main"}

As you see it is difficult to distinguish metrics.
Yeah, we can do it by instance label - but it is not so elegant way in case we want to have a filter (main-1 or main-2), because we need to parse extra labels from the instance label.

@rustatian
Copy link
Member

The best practice is not to define some extra parameters in Prometheus.

I don't know actually about what practices you are talking about. Would be great to see link to them.

If you need to use these metrics in Grafana for example, this is as easy as adding {instance=~"$instance"} to the metric. So, every metric will contain needed field.

@asanikovich
Copy link
Author

@rustatian
We have mane many pods with RR and thats why some labels will help to distinguish them.

@rustatian
Copy link
Member

Let's see the community respond on that. Because personally I'm not sure, that this case should be solved on the RR side. However, few notes about the proposal:

metrics:
  labels:
    - env: "APP_ENV"
    - app: "APP_NAME"

You may specify env variables in the configuration w/o specifying the env key. Like that for example:

metrics:
  labels:
    - env: ${SOME_ENV:-default_value}
    - app: ${SECOND_ENV}

:- this is funny, but official syntax for the default parameter expansion (supported by RR as well): https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html

@asanikovich
Copy link
Author

@rustatian This format will be good

@asanikovich
Copy link
Author

@rustatian Hello. Any news?

@rustatian
Copy link
Member

Hey @asanikovich 馃憢馃徎
Unfortunately no one is interested in this feature (0 upvotes). I have moved it to low priority. But, as always, I would be happy to review (or help with) a PR 馃槂

  1. Labels are defined here: https://github.com/roadrunner-server/metrics/blob/master/config.go#L54C5-L54C5. They are defined per-collector: https://github.com/roadrunner-server/metrics/blob/master/config.go#L27 (you need to choose the right one, I guess you need a counter)
  2. Then, you need to validate those labels here: https://github.com/roadrunner-server/metrics/blob/master/config.go#L69. By check I mean register. But before, since your proposal about env variables, you have to expand these envs: https://github.com/roadrunner-server/config/blob/master/expand.go
  3. Final step - register your metric.

P.S: Since the expand algorithm should be used in two places, to DRY, I suggest you to create a folder named expand in our SDK: https://github.com/roadrunner-server/sdk and put the algorithm in it (also move it from the config).

@rustatian rustatian added the help-needed-easy Call for participation: Experience needed to fix: Easy / not much label Dec 25, 2023
@mtamazlicaru
Copy link

Hi @rustatian. I would be interested to help with this feature.
Is the feature still relevant?

@rustatian
Copy link
Member

Hey @mtamazlicaru 馃憢
Definitely, if you need any help from my side, feel free to ask (here or on our Discord server).

@mtamazlicaru
Copy link

@rustatian I have a question regarding first point from #1632 (comment), how I understand it labels that are mentioned there are per application metrics.

Since the suggestion was to have generic labels for all metrics, I think labels should be added in this struct.

What is your opinion on this? Thank you.

@rustatian
Copy link
Member

Hey @mtamazlicaru 馃憢
Yes, they should be added as a top-level key in the Config structure and then spread across all registered Collectors. Keep in mind, that the user can register metrics in runtime via RPC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: feature requested, but need to be discussed help-needed-easy Call for participation: Experience needed to fix: Easy / not much
Projects
Status: 馃啎 New
Development

No branches or pull requests

3 participants