open-telemetry / semantic-conventions

Defines standards for generating consistent, accessible telemetry across a variety of domains
Apache License 2.0
274 stars 175 forks source link

Should metric namespaces be restricted from also being metric names? #50

Open trask opened 1 year ago

trask commented 1 year ago

Summarizing reasons for not introducing this restriction:

Summarizing reasons for introducing this restriction:

I will update these summaries based on any comments made below.

trask commented 1 year ago

If "Option C" is chosen in https://github.com/open-telemetry/semantic-conventions/issues/51#issuecomment-1561642471, I think that this reason not introducing the restriction will no longer be relevant:

because system.processes.count has attribute state, which would implicitly become system.processes.state, while if the metric was system.processes, the attribute state would implicitly become system.state.

reyang commented 1 year ago

The TC voted and here is the result https://github.com/open-telemetry/semantic-conventions/issues/51#issuecomment-1607892958.

trask commented 1 year ago

@reyang this issue is about namespaces for instrument names as opposed to namespaces for attribute names, did the TC vote also on this? thx

Oberon00 commented 1 year ago

To me this looks like a mix-up. Re-opening.

trask commented 1 year ago

On second look, maybe the decision has been made(?), based on https://github.com/open-telemetry/semantic-conventions/issues/50#issuecomment-1564648535

If "Option C" is chosen in https://github.com/open-telemetry/semantic-conventions/issues/51#issuecomment-1561642471, I think that this reason not introducing the restriction will no longer be relevant:

  • allows dropping .count suffixes for more concise metric names, e.g. system.processes instead of system.processes.count

because system.processes.count has attribute state, which would implicitly become system.processes.state, while if the metric was system.processes, the attribute state would implicitly become system.state.

trask commented 1 year ago

Or, more precisely, https://github.com/open-telemetry/semantic-conventions/issues/51#issuecomment-1607892958 effectively decides the underlying question that led to this issue (naming of system.processes.count and jvm.threads.count) and so this specific issue is no longer blocking progress on JVM semantic conventions.

trask commented 1 year ago

just a note, I did find something in our current conventions which is both a metric and a metric name: hw.voltage

braydonk commented 1 year ago

I have a scenario in #330 that shows an example of how not introducing this restriction would lead to a clearer naming outcome.

330 is set to change the process schema by namespacing all attributes pursuant to the decision in #51 (this is partially just to get the change out of the way, but also to work around limitations of the semconv yaml tooling with regards to duplicates in the yaml spec). There were 2 attribute naming scenarios that would become problematic if the decision in this issue is to introduce the restriction (i.e. metric names cannot be namespaces).

For example, two metrics that posed a problem were process.network.io and process.disk.io; both of these metrics have an attribute called direction, but it cannot be shared. direction on process.disk.io has the set labels read and write, direction on process.network.io has the set labels receive and transmit. It makes sense for direction to not be made into a common metric so that these set label values make more sense semantically with the metric they are on (i.e. read and write aren't the nomenclature one would use to describe network IO and vice versa).

In the PR, I've named these attributes process.disk.io.direction and process.network.io.direction respectively. I thought this made sense for two reasons:

If this restriction was put in place, making the process.disk.io.direction name illegal since process.disk.io is the metric name and thus cannot be a namespace, the attribute would need to be renamed process.disk.io_direction. This is okay, but I think it's worse than process.disk.io.direction since the name of the attribute no longer reveals its exclusive membership of that metric, and instead only to the process.disk namespace. (This is a namespace with nothing else in it, which may or may not apply to other similar scenarios, but applies to every scenario in the process metrics PR.)

process.disk.io_direction, process.network.io_direction, process.paging.fault_type, and process.context_switch_type are all the attribute names that would be necessary if the restriction was in place. All of these attributes are exclusive to their respective metrics, and thus I think them being in the namespace of the metric is more ergonomic than the naming gymnastics necessary just to keep them out of the metric namespace.

trask commented 1 year ago

If this restriction was put in place, making the process.disk.io.direction name illegal since process.disk.io is the metric name and thus cannot be a namespace, the attribute would need to be renamed process.disk.io_direction

I believe this issue is only about metric namespaces and metric names (and not related to attribute namespaces or attribute names).

joaopgrassi commented 1 year ago

For example, two metrics that posed a problem were process.network.io and process.disk.io; both of these metrics have an attribute called direction, but it cannot be shared. direction on process.disk.io has the set labels read and write, direction on process.network.io has the set labels receive and transmit. It makes sense for direction to not be made into a common metric so that these set label values make more sense semantically with the metric they are on (i.e. read and write aren't the nomenclature one would use to describe network IO and vice versa).

@braydonk For those, the direction attribute goes on the namespace, process.network.* and process.disk.io, like I did for the system metrics: https://github.com/open-telemetry/semantic-conventions/blob/main/docs/system/system-metrics.md#metric-systemdiskio

The problem I see in #330 that I think is related to this issue is: There's a metric process.paging.faults and today, there's an attribute type for it. In the PR, the type attribute is added to the metric namespace, becoming process.paging.faults.type. The "problem" is that maybe type under process.paging.* makes no sense.

My point being: if this issue gets solved with: metric names cannot be also namespaces, then #330 is invalid.

braydonk commented 1 year ago

I believe this issue is only about metric namespaces and metric names (and not related to attribute namespaces or attribute names).

I was unaware there was a distinction. So the decision in this issue wouldn't affect process.disk.io for example from being used as an attribute namespace?

For those, the direction attribute goes on the namespace, process.network.* and process.disk.io

That is what I'm trying to challenge. I think process.disk.io.direction is a better name for the attribute, and why I want to understand the decision in this issue and whether it means process.disk.io.direction is an illegal attribute name.

trask commented 2 months ago

this has come up in https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/11901 which is proposing metrics: