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.

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

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

Open
trask opened this issue May 24, 2023 · 12 comments
Open

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

trask opened this issue May 24, 2023 · 12 comments
Assignees

Comments

@trask
Copy link
Member

trask commented May 24, 2023

Summarizing reasons for not introducing this restriction:

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

Summarizing reasons for introducing this restriction:

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

@trask
Copy link
Member Author

trask commented May 26, 2023

If "Option C" is chosen in #51 (comment), 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.

@reyang
Copy link
Member

reyang commented Jul 3, 2023

The TC voted and here is the result #51 (comment).

@reyang reyang closed this as completed Jul 3, 2023
@trask
Copy link
Member Author

trask commented Jul 3, 2023

@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
Copy link
Member

Oberon00 commented Jul 4, 2023

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

@trask
Copy link
Member Author

trask commented Jul 25, 2023

On second look, maybe the decision has been made(?), based on #50 (comment)

If "Option C" is chosen in #51 (comment), 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
Copy link
Member Author

trask commented Jul 25, 2023

Or, more precisely, #51 (comment) 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
Copy link
Member Author

trask commented Aug 15, 2023

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

@braydonk
Copy link
Contributor

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:

  • It is made clear that these attributes both belong to each respective metric exclusively. There is no scenario where these attributes would be used anywhere other than in these metrics
  • The names make sense when read.

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
Copy link
Member Author

trask commented Oct 12, 2023

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
Copy link
Member

joaopgrassi commented Oct 13, 2023

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
Copy link
Contributor

braydonk commented Oct 13, 2023

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
Copy link
Member Author

trask commented Aug 21, 2024

this has come up in open-telemetry/opentelemetry-java-instrumentation#11901 which is proposing metrics:

  • camel.context.exchange
  • camel.context.exchange.completed
  • camel.context.exchange.inflight
  • camel.context.exchange.failed
  • camel.context.exchange.failed.handled
  • camel.context.exchange.redelivered
  • camel.context.exchange.redelivered.external

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants