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

Metric namespaces should not be pluralized #267

Merged
merged 4 commits into from
Oct 18, 2023

Conversation

trask
Copy link
Member

@trask trask commented Aug 17, 2023

Fixes #212

Changes

Metric namespaces SHOULD NOT be pluralized.

This affect these current metric namespaces:

Merge requirement checklist

@trask trask force-pushed the metric-namespace-pluralization branch from fe2e8a0 to 8afc168 Compare August 17, 2023 15:55
@trask trask marked this pull request as ready for review August 17, 2023 15:55
@trask trask requested review from a team August 17, 2023 15:55
@joaopgrassi
Copy link
Member

joaopgrassi commented Aug 21, 2023

Just want to bring this comment from @dmitryax from #212 (comment)

I'd also suggest bringing metrics currently emitted by Collector receivers but not defined in the semantic conventions. There must be plenty of them with pluralized namespaces. Maybe they can influence this decision. I can do that exercise if it makes sense

I think we probably should do that before moving forward with this? If that has a large impact maybe we should reconsider things? CC @trask @jsuereth

@jsuereth
Copy link
Contributor

@joaopgrassi @mx-psi I don't think removing pluralization will be reconsidered (it actually already happened in the specification), but what we WOULD do is figure out how to provide soft/easy migraiton for users and avoid continual churn of this decision.

@joaopgrassi
Copy link
Member

@jsuereth sounds good!

For the metrics we have today in the conventions I think we are fine, I'm just a bit worried with the ones from the collector, as described by @dmitryax here.

I guess we will just have to come up with new names for it, and probably a general guideline for situations where removing the s from the namespace, render the new metric name confusing/useless, like redis.clients.connected -> redis.client.connected. I think this will be a recurring topic.

@jsuereth
Copy link
Contributor

@joaopgrassi I think the example you list is clearly the case where it should be redis.clients_connected or redis.server.clients_connected vs. redis.client.connection.count, i.e. i'm not sure client or clients is a viable namespace for that one, nor is it clear what it's referring to.

In other words, I think we should continue with merging this PR and work on clearing up semantics of existing metrics for clarity.

@jsuereth jsuereth merged commit 7700f5d into open-telemetry:main Oct 18, 2023
9 checks passed
@trask trask deleted the metric-namespace-pluralization branch October 14, 2024 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Proposal: don't pluralize metric namespaces
7 participants