Add specification of metric instrument units #1177

justinfoote · 2020-11-02T18:00:07Z

Fixes #705

Changes

This PR adds some requirements about units for Metric Instruments.

I made some decisions that I'd love feedback on:

We will not do any reaggregation or reconciliation of units in the SDK (this is to eliminate the need to parse unit strings or provide a more feature-rich units API, which is a long rabbit hole).
Metric instruments should assume that any unit string they get is valid.
I have left it up to API implementors to decide whether to provide constants or composition functions for the required units.
I've identified a handful of base units, along with a few prefixes that we consider important. I chose these based on the current semantic conventions, and I included a few other frequently used units. As we write more semantic conventions, we should add more units as needed.
I chose the case-insensitive form from the UCUM specification. I'm willing to be convinced that I made the wrong choice here. This is the guidance from UCUM:

Some computer systems or programming languages still have the requirement of case insensitivity and some humans who are not familiar with SI units tend to confuse upper and lower case or can not interpret the difference in upper and lower case correctly. For this reason the case insensitive symbols are defined. Although The Unified Code for Units of Measure does not encourage use of case insensitive symbols where not absolutely necessary, in some circumstances the case insensitive representation may be the greatest common denominator. Thus some systems that can handle case sensitivity may end up using case insensitive symbols in order to communicate with less powerful systems.

specification/metrics/semantic_conventions/README.md

justinfoote · 2020-11-02T21:09:53Z

specification/metrics/semantic_conventions/README.md

+Implementors may choose the correct prefix to use for the scale of values being
+measured. The choice of prefix may affect the value type to be recorded.
+
+> Example: it may be reasonable to record time as an integer when using an


@aabmass - I didn't see your comment on #705 before I wrote this up. I'd love your feedback about the wording of this section. How can we add the guidance you were suggesting (if the values to be recorded are whole numbers, instrumenters should prefer recording in a smaller unit (like nanoseconds) rather than recording a floating point in a larger unit)

I wonder if others think this is a good idea in the first place. The reason I suggested it is:

Higher precision

Potentially smaller on wire if we ever switched OTLP to varint

However, Prometheus recommends base units (seconds) and statsd uses milliseconds for time. Is the gain in precision worth it over sticking to prior art? There is also a concern of overflow for very large values, but I can't think of an actual example that would overflow.

specification/metrics/semantic_conventions/README.md

specification/metrics/api.md

jmacd · 2020-11-12T06:53:19Z

specification/metrics/api.md

+
+Metric instruments SHOULD NOT validate that the unit string passed to them is a
+valid value. The default unit to be used by a metric instrument when unit is not
+provided SHOULD be the non-annotated unity unit (that is: `1`).


I worry a bit about the use of units jargon that may be lost on our readers. On the other hand, I want to embrace it. We should introduce terms to explain the syntax, like "{thing}" is a unity unit counting a quanity of things, to start with, so that a non-annotated unity unit is what you get when all you know is "quantity of things".

Also in the UCUM document, the terms "metric" vs "nonmetric". I'm afraid we also use the term "metric", so this overloading can be confusing, but it's also quite handy.

Some of the lingering confusion about the choice between ValueRecorder/ValueObserver (a.k.a. "Grouping" instruments) vs the others (a.k.a. "Adding" instruments) can be reflected in terms used in the UCUM document.

For example,

Furthermore, for a unit to be metric it must be a quantity on a ratio scale where multiplication and division with scalars are defined.

I think we can say you SHOULD NOT use a nonmetric unit with an Adding instrument.

Those symbols that are used as units that imply a measurement on a scale other than a ratio scale (e.g., interval scale, logarithmic scale) are defined differently. These do not represent proper units as elements of the group (U,·). Therefore those special semantic entities are called special units, as opposed to proper units.

We could say that ValueRecorder/ValueObserver may be used with metric and nonmetric units, on the other hand, and the API has no way to say which it is. We might say that when you use a Grouping instrument for inputs with known-proper units, we can always correctly aggregate these using a sum (because they are "metric"). The SDK can automatically switch between sum and histogram aggregation when units are proper.

Just encouraging you to add a bit more , if you're up to it. 😀

OK, after giving this some thought, I think I'm in favor of less jargon.

I want to remove the term Unity. In the semantic convention table, I'd prefer we use the name none, to indicate that this is an instrument measuring something unitless.

I don't love the idea of introducing a new definition of the term metric. In this case, it means a unit that can be meaningfully scaled, especially by orders of magnitude. (They bring up the example that celsius is not metric, even though the SI allows metric prefixes to be used with it, because, for example, 30 celsius is not 3 times hotter than 10 celsius.) This definition is so enormously different from those used elsewhere in the spec, I think it's inviting misunderstanding.

While I could certainly see the convenience of an SDK choosing the appropriate aggregator based on the units of the instrument, I fear that it's putting too much meaning into a value that I don't think the SDK should need to reason about. There is a lot of meaning that our SDKs could parse out of the units field, but it's a slippery slope. This is part of why this PR took me so long to write -- I kept adding a whole bunch of complexity, then I'd have to scale it back and delete a bunch of paragraphs.

Having said all that, I would be in favor of adding guidance about what sort of units to use with adding or grouping instruments; I think that would be useful. I'll try putting something like that together today.

How about "identity unit"? "unitless unit"?

👍 I think I prefer unitless.

I went with unitless for now. I'm open to more feedback about it. Naming things is hard.

justinfoote · 2020-11-20T16:34:29Z

Hey, anyone who's interested in seeing this PR merged, I'd love to get my build-tools PR reviewed. It generates markdown and code for these units. Find it here: open-telemetry/build-tools#21

github-actions · 2020-11-28T03:23:13Z

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

jgals · 2020-12-04T16:18:03Z

@justinfoote and @jmacd, I'm commenting here to avoid having this PR closed by the bot. 🤖

jkwatson · 2020-12-04T19:16:27Z

specification/metrics/api.md

+| operations  | operations               | {operations}  |
+| packets     | packets                  | {packets}     |
+| processes   | processes                | {processes}   |
+| threads     | threads                  | {threads}     |


would it make sense to put these into some yaml so the could be auto-generated to support them?

(as a TODO?)

💯 There is a corresponding PR here: open-telemetry/build-tools#21

I had considered adding the yaml to this PR according to that proposal, but I thought it more prudent to wait until that PR merges. It's looking like that's imminent, so maybe I'll go ahead and add the yaml now.

I added the yaml and the generated markdown table in 6fe5382.
This introduces a hard dependency between these PRs though. We can't merge this one until the build-tools one is merged.

spec-compliance-matrix.md

jmacd · 2020-12-11T18:16:27Z

specification/metrics/api.md

+
+Metric instruments SHOULD NOT validate that the unit string passed to them is a
+valid value. The default unit to be used by a metric instrument when unit is not
+provided SHOULD be the non-annotated unity unit (that is: `1`).


How about "identity unit"? "unitless unit"?

In an effort to reduce the unit-related jargon introduced in this part of the spec.

github-actions · 2020-12-22T03:38:31Z

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

github-actions · 2021-01-12T03:56:33Z

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

jmacd

Thank you @justinfoote

jmacd · 2021-01-13T17:45:47Z

@open-telemetry/specs-metrics-approvers please review.

github-actions · 2021-01-21T04:09:06Z

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

github-actions · 2021-02-04T03:26:45Z

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

github-actions · 2021-02-11T03:27:42Z

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

weyert · 2021-03-25T02:29:02Z

@jmacd @justinfoote @open-telemetry/specs-metrics-approvers Was it intended to close this PR?

jmacd · 2021-03-25T06:12:16Z

@weyert I ❤️ this PR and want to get it approved.

github-actions · 2021-04-03T03:19:35Z

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

github-actions · 2021-04-11T03:19:23Z

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

Oberon00 · 2021-09-23T09:27:54Z

In the context of open-telemetry/build-tools#65, this came up again. Do you still plan to merge this? If so, would someone care to update the generator docs to document this "unit" feature?

Add specification of metric instrument units

86acef4

justinfoote requested review from a team November 2, 2020 18:00

github-actions bot assigned jmacd Nov 2, 2020

Add metric instrument unit spec to changelog

369d9e2

MrAlias approved these changes Nov 2, 2020

View reviewed changes

specification/metrics/semantic_conventions/README.md Show resolved Hide resolved

Add metric instrument units to spec-compliance-matrix

07094f5

justinfoote commented Nov 2, 2020

View reviewed changes

aabmass reviewed Nov 4, 2020

View reviewed changes

specification/metrics/semantic_conventions/README.md Outdated Show resolved Hide resolved

justinfoote mentioned this pull request Nov 5, 2020

Add unit semantic convention generator open-telemetry/build-tools#21

Merged

justinfoote added 2 commits November 5, 2020 12:28

Remove contradictory paragraph about metric unit constants

c151496

Add unity unit

33921fe

jmacd reviewed Nov 12, 2020

View reviewed changes

jmacd mentioned this pull request Nov 12, 2020

Add semantic convention for timing durations #1142

Closed

github-actions bot added the Stale label Nov 28, 2020

MrAlias removed the Stale label Nov 30, 2020

jkwatson reviewed Dec 4, 2020

View reviewed changes

jmacd reviewed Dec 11, 2020

View reviewed changes

justinfoote added 3 commits December 11, 2020 12:45

Recommend lowercase unit strings

2ba63c9

Add metric accumulator unit-based aggregation to compliance matrix

1130c9c

Replace "unity" with unitless in metrics semantic conventions.

09a4a61

In an effort to reduce the unit-related jargon introduced in this part of the spec.

justinfoote force-pushed the metric_unit_spec branch from 257c961 to 09a4a61 Compare December 11, 2020 20:58

justinfoote added 2 commits December 11, 2020 13:10

Add metric units yaml; generated units markdown table

6fe5382

Merge branch 'master' into metric_unit_spec

9e2c011

github-actions bot added the Stale label Dec 22, 2020

justinfoote requested review from jkwatson, jmacd and aabmass January 4, 2021 18:02

github-actions bot removed the Stale label Jan 5, 2021

github-actions bot added Stale and removed Stale labels Jan 12, 2021

jmacd approved these changes Jan 13, 2021

View reviewed changes

github-actions bot added the Stale label Jan 21, 2021

arminru requested a review from a team January 21, 2021 17:53

jmacd removed the Stale label Jan 21, 2021

Base automatically changed from master to main January 27, 2021 21:16

github-actions bot added the Stale label Feb 4, 2021

github-actions bot closed this Feb 11, 2021

jmacd reopened this Mar 25, 2021

reyang mentioned this pull request Mar 25, 2021

Adding the Metrics Counter API #1578

Merged

github-actions bot removed the Stale label Mar 26, 2021

github-actions bot added the Stale label Apr 3, 2021

github-actions bot closed this Apr 11, 2021

sl0thentr0py mentioned this pull request Mar 8, 2022

feat(measurements): Add new measurements section to transaction getsentry/develop#515

Merged

fatsheep9146 mentioned this pull request Oct 9, 2022

[otelgrpc] add metric rpc.server.duration support open-telemetry/opentelemetry-go-contrib#2700

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add specification of metric instrument units #1177

Add specification of metric instrument units #1177

justinfoote commented Nov 2, 2020 •

edited

Loading

justinfoote Nov 2, 2020

aabmass Nov 4, 2020

jmacd Nov 12, 2020

justinfoote Nov 20, 2020

jmacd Dec 11, 2020

justinfoote Dec 11, 2020

justinfoote Dec 11, 2020

justinfoote commented Nov 20, 2020

github-actions bot commented Nov 28, 2020

jgals commented Dec 4, 2020

jkwatson Dec 4, 2020

jmacd Dec 11, 2020

justinfoote Dec 11, 2020

justinfoote Dec 11, 2020

jmacd Dec 11, 2020

github-actions bot commented Dec 22, 2020

github-actions bot commented Jan 12, 2021

jmacd left a comment

jmacd commented Jan 13, 2021

github-actions bot commented Jan 21, 2021

github-actions bot commented Feb 4, 2021

github-actions bot commented Feb 11, 2021

weyert commented Mar 25, 2021 •

edited

Loading

jmacd commented Mar 25, 2021

github-actions bot commented Apr 3, 2021

github-actions bot commented Apr 11, 2021

Oberon00 commented Sep 23, 2021

Add specification of metric instrument units #1177

Add specification of metric instrument units #1177

Conversation

justinfoote commented Nov 2, 2020 • edited Loading

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinfoote commented Nov 20, 2020

github-actions bot commented Nov 28, 2020

jgals commented Dec 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 22, 2020

github-actions bot commented Jan 12, 2021

jmacd left a comment

Choose a reason for hiding this comment

jmacd commented Jan 13, 2021

github-actions bot commented Jan 21, 2021

github-actions bot commented Feb 4, 2021

github-actions bot commented Feb 11, 2021

weyert commented Mar 25, 2021 • edited Loading

jmacd commented Mar 25, 2021

github-actions bot commented Apr 3, 2021

github-actions bot commented Apr 11, 2021

Oberon00 commented Sep 23, 2021

justinfoote commented Nov 2, 2020 •

edited

Loading

weyert commented Mar 25, 2021 •

edited

Loading