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

Clarification: metric namespaces are not allowed to be metric names #3476

Conversation

trask
Copy link
Member

@trask trask commented May 5, 2023

Related to #3457 (comment)

Note that this is the same as attribute namespaces which are not allowed to coincide with attribute names:

Names SHOULD NOT coincide with namespaces. For example if service.instance.id is an attribute name then it is no longer valid to have an attribute named service.instance because service.instance is already a namespace. Because of this rule be careful when choosing names: every existing name prohibits existence of an equally named namespace in the future, and vice versa: any existing namespace prohibits existence of an equally named attribute key in the future.

See alternative proposal (that metric namespaces ARE allowed to be metric names) at #3477

Changes

Clarifies that metric namespaces are not allowed to be metric names.

@trask trask force-pushed the clarification-metric-namespaces-are-not-allowed-to-be-metric-names branch from 21aecc4 to fc1c002 Compare May 5, 2023 18:52
@trask trask marked this pull request as ready for review May 5, 2023 18:55
@trask trask requested review from a team May 5, 2023 18:55
@dyladan
Copy link
Member

dyladan commented May 5, 2023

Seems like a single issue with an emoji voting system would be an easier way to compare 2 proposals. In any case I thumbed up the other one

@trask
Copy link
Member Author

trask commented May 5, 2023

I thumbed up the other one

can you approve it instead (or in addition)?

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any reasoning one way or another, so I support this approach purely for consistency reasons.

@trask
Copy link
Member Author

trask commented May 8, 2023

I don't see any reasoning one way or another, so I support this approach purely for consistency reasons.

yeah, I'm not aware of (and couldn't find) what the reasoning is for attribute namespaces either, does anyone know?

@reyang reyang added the area:semantic-conventions Related to semantic conventions label May 9, 2023
@reyang
Copy link
Member

reyang commented May 9, 2023

@trask heads up - most likely this PR will be closed, and we'll ask you to resubmit the PR in a new repo, please refer to #3474 (comment).

@pichlermarc
Copy link
Member

I think this will require us to keep the recommending the .count suffix or something of the like. As outlined in #3457, I don't think that .total is an adequate suffix for delta metrics which would be the replacement. Therefore I support the other proposal #3477

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This restriction has generally been followed in the collector when defining metrics collected by "scraper" receivers.

I think there is at least some utility in this restriction. For example, one could autogenerate navigable documentation based on metric namespaces, where each page would contain either a list of "sub" namespaces, or metadata for a specific metric. Allowing collisions between namespaces and metrics would be manageable by having a page that shows both a metric and a list of "sub" namespaces. However, I think as a user traversing such documentation, I might find it a bit counterintuitive.

@trask
Copy link
Member Author

trask commented May 9, 2023

I think there is at least some utility in this restriction. For example, one could autogenerate navigable documentation based on metric namespaces

this could apply to some metric UIs also

Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tigrannajaryan
Copy link
Member

tigrannajaryan commented May 19, 2023

I found the initial PR that added the limitation for attribute names: #832 [UPDATE: wrong PR, trying to find the original, git blame is not helping]

[UPDATE 2] this is the PR that introduced the limitation, but there is no discussion about why: #807 I suspect it may have happened in SIG meeting, can't recall anymore. The only argument that I was able to find saying why this limitation can be useful is the one I linked below.

See here for one argument about why the limitation: #832 (comment) (copying below)

Often the namespaces represent "things". For example all attributes in k8s.pod.* namespace are about Kubernetes Pod. If we allow an attribute name that coincides with a namespace it appears that the value of the attribute describes the "thing". However, in this case why is it not just another attribute under that namespace? What would be a reasonable value for k8s.pod attribute that does not have a better name in the k8s.pod.* namespace?
I think if attributes like that existed it would be a source of confusion. Does service.instance attribute describe something about service (as a "thing") or something about service.instance (which is a different "thing")?

The requirement I added prevents this confusion. When attributes are used for "things" it is very clear which "thing" they are about, it is the "thing" denoted by their namespace (whereas if we allowed what you suggest it could be also the "thing" denoted by the parent namespace).

@djaglowski
Copy link
Member

djaglowski commented May 23, 2023

Is it controversial that we should be able to convert bidirectionally between our dot-notation and common structured data formats such as json or yaml? If not, this would seem to be a clear reason to keep metric names and namespaces separate. e.g.

metrics:
  hello.foo: 1
  hello.bar: 2

# equivalent to
metrics:
  hello:
    foo: 1
    bar: 2

If metrics names and namespaces are allowed to collide, this is problematic:

metrics:
  hello: 1
  hello.bar: 2

# equivalent?
metrics:
  hello: # is this the value '1' or an object containing "bar: 2"?

Edit: Here is some documentation from ECS regarding the equivalency of these formats.

@trask
Copy link
Member Author

trask commented May 24, 2023

hey all, the @open-telemetry/technical-committee is reviewing this issue, and asked to create a summary of pros/cons. I have created open-telemetry/semantic-conventions#50 based on the comments here, please comment further over there if I missed anything, thx!

@trask trask closed this May 24, 2023
@trask trask deleted the clarification-metric-namespaces-are-not-allowed-to-be-metric-names branch May 24, 2023 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants