Add some clarifying language to the semantics of metric instrument naming #552
Conversation
jkwatson
requested review from
arminru,
bogdandrutu,
c24t,
carlosalberto,
iredelmeier,
jmacd,
reyang,
SergeyKanzhelev,
tedsuo,
tigrannajaryan and
yurishkuro
as code owners
specification/metrics/api-user.md
Outdated
| Instead, favor a name like "http_request_latency", as it informs the viewer of the | ||
| semantic meaning of the latency. |
There was a problem hiding this comment.
Just add an extra comment to say that this is an example and the list of standard names will be defined separately. I try to avoid making this example the canonical name for http_request_latency (not because it is a bad name, but because every document may include some examples and we want to have a semantic convention separate doc to keep all the names in one place).
There was a problem hiding this comment.
I think until we have semantic conventions, we need some guidance, via examples, no?
There was a problem hiding this comment.
from the spec sig mtg today, sounds like the example just needs to be touched up so the example is not interpreted as authoritative cc @bogdandrutu @jkwatson
There was a problem hiding this comment.
I updated the language in here. PTAL
|
|
||
| 1. They are non-empty strings | ||
| 2. They are case-insensitive | ||
| 3. The first character must be non-numeric, non-space, non-punctuation | ||
| 4. Subsequent characters must be belong to the alphanumeric characters, '_', '.', and '-'. | ||
|
|
||
| Metric names belong to a namespace, which is the `name` of the associated `Meter`, | ||
| Metric instrument names belong to a namespace, which is the `name` of the associated `Meter`, |
There was a problem hiding this comment.
We discussed yesterday during metrics sig about this, I think the meter name is not a namespace, more like a source. Also I heard on gitter that you want to followup on this, what do you want to do with this PR then?
There was a problem hiding this comment.
I'd like to leave this PR open; one way or another, clarification is needed.
There was a problem hiding this comment.
@bogdandrutu We adapted the spec to explicitly use the meter name as namespace a while ago after it was proposed in a SIG spec meeting since this puts the metric names into a namespace per library to avoid name clashes between libraries unaware of each other. See #391 (comment), #408 and open-telemetry/oteps#76 (comment). What has changed since then?
There was a problem hiding this comment.
@arminru do you still have concerns?
There was a problem hiding this comment.
@bogdandrutu I thought you wanted to revise this and depart from using the meter name as a namespace / collision domain entirely but maybe I was missing context or misunderstood.
I'm fine with the current wording this PR introduces and therefore had already approved it.
|
Posting @jkwatson's related notes on the Metrics SIG discussion: https://docs.google.com/document/d/1Y-Ck7NxRMEdnKueFFCVnYsdUIFeZ9EN4PpEFCkHu2-s/edit |
jkwatson
force-pushed
the
clarify_instrument_naming_semantics
branch
from
93bb22c
to
2e0a5c3
Compare
|
The failing CI check is passing for me. Anyone have any idea what's going on? |
|
ok, CI is fixed. |
|
|
||
| 1. They are non-empty strings | ||
| 2. They are case-insensitive | ||
| 3. The first character must be non-numeric, non-space, non-punctuation | ||
| 4. Subsequent characters must be belong to the alphanumeric characters, '_', '.', and '-'. | ||
|
|
||
| Metric names belong to a namespace, which is the `name` of the associated `Meter`, | ||
| Metric instrument names belong to a namespace, which is the `name` of the associated `Meter`, |
There was a problem hiding this comment.
@bogdandrutu I thought you wanted to revise this and depart from using the meter name as a namespace / collision domain entirely but maybe I was missing context or misunderstood.
I'm fine with the current wording this PR introduces and therefore had already approved it.
Should hopefully let us close #434 .