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 #107

Merged
merged 11 commits into from
Jul 25, 2023
Merged

.count metric naming convention only applies to UpDownCounters #107

merged 11 commits into from
Jul 25, 2023

Conversation

trask
Copy link
Member

@trask trask commented Jun 13, 2023 •
edited
Loading

@trask trask changed the title .count metric naming convention only applies to UpDownCounters .count metric naming convention only applies to UpDownCounters Jun 13, 2023
@trask trask marked this pull request as ready for review June 13, 2023 18:44
@trask trask requested review from a team June 13, 2023 18:44
@trask
Copy link
Member Author

trask commented Jun 14, 2023

pinging @reyang @jack-berg @jsuereth @jmacd @MrAlias @gouthamve who approved this same PR before the repo split

(and @joaopgrassi who commented on it)

gouthamve
gouthamve approved these changes Jun 15, 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

joaopgrassi
joaopgrassi approved these changes Jun 15, 2023
View reviewed changes
Copy link
Member

@joaopgrassi joaopgrassi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Approving to get this improvement in. I guess the final outcome of it will be when we have an agreement on #50.

Should we also port the old PRs #3476/#3477 here? So we can voice our preferences again over which approach?

@pirgeo pirgeo mentioned this pull request Jun 20, 2023
Closed
dashpole
dashpole approved these changes Jun 23, 2023
View reviewed changes
Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

I agree with the conclusion, with a few nits on the rationale provided

specification/metrics/semantic_conventions/README.md Outdated Show resolved Hide resolved
specification/metrics/semantic_conventions/README.md Outdated Show resolved Hide resolved
@reyang
Copy link
Member

reyang commented Jul 24, 2023

@trask would you resolve the merge conflicts? I think it's time to get this merged.

dashpole
dashpole reviewed Jul 25, 2023
View reviewed changes
docs/general/metrics.md Outdated Show resolved Hide resolved
@reyang reyang merged commit a5509c6 into open-telemetry:main Jul 25, 2023
@trask trask mentioned this pull request Jul 25, 2023
Merged
@trask trask deleted the count-v-total branch July 25, 2023 23:38
rapphil pushed a commit to rapphil/semantic-conventions that referenced this pull request Aug 14, 2023
@trask @rapphil
.count metric naming convention only applies to UpDownCounters (ope…
fe818fd 
…n-telemetry#107)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pirgeo pirgeo pirgeo left review comments

@reyang reyang reyang approved these changes

@gouthamve gouthamve gouthamve approved these changes

@joaopgrassi joaopgrassi joaopgrassi approved these changes

@MrAlias MrAlias MrAlias approved these changes

@dashpole dashpole dashpole approved these changes

@jsuereth jsuereth jsuereth approved these changes

Assignees

@arminru arminru

Labels
None yet
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 @reyang @jsuereth @dashpole @MrAlias @joaopgrassi @gouthamve @pirgeo @arminru