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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
dc75faa
count metric naming only applies to UpDownCounters
trask May 9, 2023
6bd47ec
more
trask May 9, 2023
446d458
more
trask May 9, 2023
35a8811
Update specification/metrics/semantic_conventions/README.md
trask May 9, 2023
1765a50
Update specification/metrics/semantic_conventions/README.md
trask May 9, 2023
a083fe8
Update specification/metrics/semantic_conventions/README.md
trask May 9, 2023
0626038
Update specification/metrics/semantic_conventions/README.md
trask May 9, 2023
ab96132
Update specification/metrics/semantic_conventions/README.md
trask May 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,9 @@ release.
`http.scheme` to `url.scheme`,
and removes `http.target` breaking it down to `http.target` to `url.path`, `url.query`, and `url.fragment`.
([#3355](https://github.com/open-telemetry/opentelemetry-specification/pull/3355))
- Update `.count` metric naming convention so that it only applies to UpDownCounters,
and add that `.total` should not be used by either Counters or UpDownCounters
([#3493](https://github.com/open-telemetry/opentelemetry-specification/pull/3493))

### Compatibility

Expand Down
19 changes: 17 additions & 2 deletions specification/metrics/semantic_conventions/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ linkTitle: Semantic Conventions
* [Name Reuse Prohibition](#name-reuse-prohibition)
* [Units](#units)
* [Pluralization](#pluralization)
+ [Use `count` Instead of Pluralization](#use-count-instead-of-pluralization)
+ [Use `count` Instead of Pluralization for UpDownCounters](#use-count-instead-of-pluralization-for-updowncounters)
+ [Do not use `total`](#do-not-use-total)
- [General Metric Semantic Conventions](#general-metric-semantic-conventions)
* [Instrument Naming](#instrument-naming)
* [Instrument Units](#instrument-units)
Expand Down Expand Up @@ -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.


If the value being recorded represents the count of concepts signified
by the namespace then the metric should be named `count` (within its namespace).
Expand All @@ -125,6 +126,20 @@ to the processes then to represent the count of the processes we can have a metr
`system.processes.count`. The suffix `count` here indicates that it is the count of
`system.processes`.

This rule should only be applied to UpDownCounters, since (monotonic) Counters have
trask marked this conversation as resolved.
Show resolved Hide resolved
`_total` appended to their names when they are [mapped to Prometheus](../../compatibility/prometheus_and_openmetrics.md#otlp-metric-points-to-prometheus), which would lead to
`_count_total`.

#### Do not use `total`

Counters have `_total` appended to their names when they are [mapped to Prometheus](../../compatibility/prometheus_and_openmetrics.md#otlp-metric-points-to-prometheus),
trask marked this conversation as resolved.
Show resolved Hide resolved
which would lead to `_total_total`. The reason that the Prometheus mapping cannot
suppress adding the duplicate `_total` is because then the mapping wouldn't be
bidirectional.

UpDownCounters should not use `_total` either because then they will look like
trask marked this conversation as resolved.
Show resolved Hide resolved
monotonic sums in Prometheus.

## General Metric Semantic Conventions

**Status**: [Mixed](../../document-status.md)
Expand Down