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.

Sign up for GitHub

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

Jump to bottom

fix(metrics)!: rename mercure_subscribers to mercure_subscribers_connected #725

Merged
merged 3 commits into from
Jan 10, 2023
Merged

fix(metrics)!: rename mercure_subscribers to mercure_subscribers_connected #725

merged 3 commits into from
Jan 10, 2023

Conversation

ndousson
Copy link
Contributor

This change allow Datadog agent to fetch all metrics starting with mercure_subscribers

  • mercure_subscribers_gauge (previously mercure_subscribers)
  • mercure_subscribers_total

See #720 issue for more information

@CLAassistant
Copy link

CLAassistant commented Dec 28, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

dunglas
dunglas approved these changes Dec 28, 2022
View reviewed changes
Copy link
Owner

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

LGTM

@ndousson
Copy link
Contributor Author

@dunglas I changed to mercure_subscribers_in_flight as _gauge suffix is not accepted by the linter

@dunglas
Copy link
Owner

dunglas commented Dec 28, 2022

Which linter was failing? Are they some open metrics / Prometheus best practices about naming?

@ndousson
Copy link
Contributor Author

ndousson commented Dec 28, 2022 •
edited
Loading

It was promlinter:

metrics.go:50:4: Metric: mercure_subscribers_gauge Error: metric name should not include type 'gauge' (promlinter)

Prometheus have some best practices but didn't prohibit _gauge suffix:

https://prometheus.io/docs/practices/naming/

OpenMetrics have more "strict" conventional naming:

https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#suffixes

They didn't add a suffix for gauge type but it's not forbidden.

@dunglas
Copy link
Owner

dunglas commented Dec 28, 2022

Ok thanks! mercure_connected_subscribers maybe?

@ndousson
Copy link
Contributor Author

ndousson commented Dec 28, 2022 •
edited
Loading

Sounds good, so what do you think about mercure_subscribers_connected ?
It'll allow to scope on both mercure_subscribers_metric ?

@dunglas
Copy link
Owner

dunglas commented Dec 28, 2022

LGTM

@ndousson
Copy link
Contributor Author

I let you do the merge 😉
The change will be released with the next version 🙏

@dunglas dunglas changed the title spec: Rename mercure_subscribers metrics to mercure_subscribers_gauge fix!: Rename mercure_subscribers metrics to mercure_subscribers_gauge Jan 10, 2023
@dunglas dunglas changed the title fix!: Rename mercure_subscribers metrics to mercure_subscribers_gauge fix!: rename mercure_subscribers metrics to mercure_subscribers_gauge Jan 10, 2023
@dunglas dunglas changed the title fix!: rename mercure_subscribers metrics to mercure_subscribers_gauge fix!: rename mercure_subscribers metrics to mercure_subscribers_connected Jan 10, 2023
@dunglas dunglas changed the title fix!: rename mercure_subscribers metrics to mercure_subscribers_connected fix(metrics)!: rename mercure_subscribers to mercure_subscribers_connected Jan 10, 2023
@dunglas dunglas force-pushed the mercure_metric_naming branch 2 times, most recently from da74b2a to 53522c6 Compare January 10, 2023 19:57
ndousson and others added 3 commits January 10, 2023 22:50
@ndousson @dunglas
fix(metrics)!: rename mercure_subscribers to mercure_subscribers_conn…
de0168e 
…ected

This change allow Datadog agent to fetch all metrics starting with mercure_subscribers

* mercure_subscribers_connected (previously mercure_subscribers)
* mercure_subscribers_total

See dunglas#720 issue for more information
@dunglas
docs
be287a7
@dunglas
fix linter
cf8219b
@dunglas dunglas merged commit f89b670 into dunglas:main Jan 10, 2023
@dunglas
Copy link
Owner

dunglas commented Jan 10, 2023

Thank you @ndousson!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dunglas dunglas dunglas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ndousson @CLAassistant @dunglas