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

Support metric labels (prometheus). #215

Merged
merged 6 commits into from
Feb 20, 2025
Merged

Conversation

bgrozev
Copy link
Member

@bgrozev bgrozev commented Feb 18, 2025

  • feat: Support labels for CounterMetric.
  • feat: Add labels support to DoubleGaugeMetric.
  • feat: Add labels support to LongGaugeMetric.
  • feat: Add labels support to BooleanMetric.
  • feat: Add labels support to InfoMetric.

@JonathanLennox
Copy link
Member

Is it really useful for the labeled and non-labeled versions of the metrics to be the same classes? They seem like two operations with disjoint functionality. I.e., if you use labels, you must not pass an initial value, you must not use get(), you must not convert to JSON, etc. Wouldn't it be easier and safer just to have separate labeled metric classes that don't have those APIs?

@bgrozev
Copy link
Member Author

bgrozev commented Feb 19, 2025

Is it really useful for the labeled and non-labeled versions of the metrics to be the same classes? They seem like two operations with disjoint functionality. I.e., if you use labels, you must not pass an initial value, you must not use get(), you must not convert to JSON, etc. Wouldn't it be easier and safer just to have separate labeled metric classes that don't have those APIs?

You just have to use labels consistently (match the number of labels) in the constructor and getters/setters. DoubleGaugeMetric is 143 lines in the PR, we'd have to split it in two classes of 110 (with labels) and 83 (no labels) lines. I think that's too much duplication.

@bgrozev bgrozev merged commit 43a2034 into jitsi:master Feb 20, 2025
5 checks passed
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.

3 participants