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

.count metric naming convention only applies to UpDownCounters #3493

Closed
wants to merge 8 commits into from
Closed

.count metric naming convention only applies to UpDownCounters #3493

wants to merge 8 commits into from

Conversation

trask
Copy link
Member

@trask trask commented May 9, 2023 •
edited
Loading

Fixes #3457

Changes

Updates .count metric naming convention so that it only applies to UpDownCounters, and adds that .total should not be used by either Counters or UpDownCounters

@trask trask force-pushed the count-v-total branch 2 times, most recently from 71d6af9 to 45c2c20 Compare May 9, 2023 22:18
@trask trask mentioned this pull request May 9, 2023
Closed
@trask trask mentioned this pull request May 9, 2023
Closed
reyang
reyang approved these changes May 9, 2023
View reviewed changes
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM, consider adding link to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/prometheus_and_openmetrics.md#otlp-metric-points-to-prometheus.

@trask trask marked this pull request as ready for review May 9, 2023 22:51
@trask trask requested review from a team May 9, 2023 22:51
@reyang reyang added the area:semantic-conventions Related to semantic conventions label May 9, 2023
jack-berg
jack-berg reviewed May 9, 2023
View reviewed changes
specification/metrics/semantic_conventions/README.md
@@ -114,7 +115,7 @@ should not be pluralized, even if many data points are recorded.
* `system.paging.faults`, `system.disk.operations`, and `system.network.packets`
should be pluralized, even if only a single data point is recorded.

#### Use `count` Instead of Pluralization
#### Use `count` Instead of Pluralization for UpDownCounters
Copy link
Member

Choose a reason for hiding this comment

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

Why keep this around for UpDownCounters? This section appears to include the only mentions of a metrics namespace. Getting rid this rule altogether would mean getting rid of a confusing and poorly defined concept.

Copy link
Member

Choose a reason for hiding this comment

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

Oops I found namespace mentioned in the General Guidelines section as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Getting rid this rule altogether would mean getting rid of a confusing and poorly defined concept.

I'd suggest let's start with this more obvious fix, and revisit removing the rule altogether depending on the outcome of #3476/#3477 (there are good arguments on both sides of that debate currently)

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe we decide which approach to take with the namespace, then we might not even need this? 🤔. I agree with @jack-berg I find the rules around this very confusing.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should address this separately. Approved and voted for #3477.

gouthamve
gouthamve approved these changes May 10, 2023
View reviewed changes
Copy link
Member

@gouthamve gouthamve left a comment

Choose a reason for hiding this comment

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

LGTM

jsuereth
jsuereth approved these changes May 10, 2023
View reviewed changes
Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

This is a step in the right direction.

I'm supportive of this. I like what @jack-berg suggests, but I'm also a fan of merging this as-is and going further in the future.

jsuereth
jsuereth requested changes May 11, 2023
View reviewed changes
Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

This PR needs to move to the Semantic Conventions repository.

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label May 19, 2023
@github-actions
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this May 27, 2023
@trask trask deleted the count-v-total branch June 13, 2023 18:27
@trask trask mentioned this pull request Jun 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jack-berg jack-berg jack-berg approved these changes

@reyang reyang reyang approved these changes

@joaopgrassi joaopgrassi joaopgrassi left review comments

@jsuereth jsuereth jsuereth requested changes

@gouthamve gouthamve gouthamve approved these changes

@MrAlias MrAlias MrAlias approved these changes

@jmacd jmacd jmacd approved these changes

Assignees

@yurishkuro yurishkuro

Labels
area:semantic-conventions Related to semantic conventions Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use total instead of count in the metrics semantic conventions
9 participants
@trask @jsuereth @jmacd @MrAlias @joaopgrassi @gouthamve @reyang @jack-berg @yurishkuro