opentelemetry-specification icon indicating copy to clipboard operation
opentelemetry-specification copied to clipboard

Add the short_name scope attribute

Open dashpole opened this issue 2 years ago • 10 comments

Fixes https://github.com/open-telemetry/opentelemetry-specification/issues/2682

Changes

Add the short_name scope attribute. Alternatively, it could have been added as otel.short_name to distinguish it from non-otel attributes, or prometheus.namespace to make it explicitly associated with prefixing prometheus metrics.

@jmacd @tigrannajaryan @jsuereth

dashpole avatar Jul 28 '22 20:07 dashpole

I believe it would make things more streamlined if first tackle the addition of "scope" as a semantic convention concept separately from the Prometheus use-cases/changes. So, a PR only for adding the scope new yaml/md.

Done

dashpole avatar Jul 29 '22 13:07 dashpole

@dashpole you said here that

I think the answer should be yes (namespaces are different), although we should try and avoid unnecessary collisions where possible to reduce confusion. I believe this is the same answer as the separation between resource vs trace/metric/log attributes.

If the Scope has a different namespace then why are we prefixing the attribute names by scope.? That seems unnecessary to me. It would be necessary if we decided that the Scope attribute names are in the same namespace as the Resource attributes and/or Trace or Metric attributes.

I am not sure what is the right approach but it seems like it needs to be one or another and then we need to consistent with that decision, which for now we don't seem to be.

tigrannajaryan avatar Jul 29 '22 14:07 tigrannajaryan

If the Scope has a different namespace then why are we prefixing the attribute names by scope.? That seems unnecessary to me. It would be necessary if we decided that the Scope attribute names are in the same namespace as the Resource attributes and/or Trace or Metric attributes.

My thinking was that some scope attributes may only apply to a particular definition of scope, such as module.path, or library.something... But I'm also having trouble coming up with examples, so maybe its a stretch. I'm happy to change it to short_name if you think it makes more sense

dashpole avatar Jul 29 '22 17:07 dashpole

I'm happy to change it to short_name if you think it makes more sense

It does look a bit weird. Virtually every other attribute we have in semantic conventions for Resources or Spans or Metrics has some sort of a prefix. We have never defined any attributes in the root namespace although I don't think the rules explicitly prohibit it.

However using a prefix scope. in a root namespace what is already asserted to be entirely allocated to the scopes looks weird too. Is there a way to come up with a different meaningful prefix that in the future may contain more than just the short_name?

On the other hand the benefit of using scope. prefix is that it mitigates the problem described in https://github.com/open-telemetry/opentelemetry-specification/issues/2535. If scope. is used as a prefix then in practice it is never going to clash with Resource or Span attribute names, so how we define the precedence will almost not matter.

Sorry, I am probably not helping much. I probably need to make up my mind first :-)

tigrannajaryan avatar Jul 29 '22 17:07 tigrannajaryan

It does look a bit weird. Virtually every other attribute we have in semantic conventions for Resources or Spans or Metrics has some sort of a prefix. We have never defined any attributes in the root namespace although I don't think the rules explicitly prohibit it.

I find strange that we have otel.scope.name and then scope.short_name. Are we completely opposed to have it under otel.scope.short_name? When I read the linked Attribute naming page above:

Attribute names that start with otel. are reserved to be defined by OpenTelemetry specification. These are typically used to express OpenTelemetry concepts in formats that don't have a corresponding concept.

It seems short_name falls into that well, no? It's like a sibling to name and version in regards of how important it is. Even so, if short_name is going to be used for https://github.com/open-telemetry/opentelemetry-specification/pull/2703. It carries more meaning than an arbitrary attribute on a scope.

Then, other scope attributes don't need any prefix, and can exist by themselves, we just happen to "list" them under the scope semantic conventions bcs it's where they should be applied. And this ties nicely with the next thing:

On the other hand the benefit of using scope. prefix is that it mitigates the problem described in https://github.com/open-telemetry/opentelemetry-specification/issues/2535

I would think it's better to not have a mixed approach when it comes to solving the question of how we "flatten" attributes. Like, either all signals have their attributes prefixed or none have. Having a prefix for scope and not for resource or span sounds weird to me, inconsistent.

joaopgrassi avatar Aug 01 '22 14:08 joaopgrassi

One topic that came up during today's spec call is the idea of making this attribute's behavior consistent across exporters. For example, prometheus adds it as a prefix to metrics, but why shouldn't other exporters do the same? Perhaps other non-OTLP exporters should also add this as a prefix. Maybe something like name_prefix would be clearer that the intention is for this to be a name prefix

dashpole avatar Aug 02 '22 15:08 dashpole

I've opted to update the PR to use the prefix-less short_name instead of scope.short_name based on discussions above.

dashpole avatar Aug 02 '22 19:08 dashpole

Regarding using scope. or otel. as a prefix: even if we use it for some attributes (like scope.short_name) we must still allow some other attributes that don't have the prefix. An example use case is the Events and Logs API, where we propose to have a Scope attribute named event.domain. I believe event.domain is a good name for an attribute and there is no need to prefix it with scope. or otel..

So, at the very least, IMO, we cannot have a hard rule that says scope. or otel. must be always a prefix for Scope attributes.

tigrannajaryan avatar Aug 03 '22 14:08 tigrannajaryan

I've opted to update the PR to use the prefix-less short_name instead of scope.short_name based on discussions above.

@dashpole does the tooling works in this case, to auto generate the table? (without a prefix)

joaopgrassi avatar Aug 05 '22 14:08 joaopgrassi

@dashpole does the tooling works in this case, to auto generate the table? (without a prefix)

I had to add one commit to the scope build-tools PR to allow an empty prefix, but otherwise it works.

dashpole avatar Aug 05 '22 15:08 dashpole

As discussed in the spec SIG today I wonder whether it would be beneficial to define a requirement that scope attributes be prefixed with otel.scope when processed by non-OTel components. This would align with otel.scope.name and otel.scope.version and provide a clear indication that an attribute was a scope attribute and not an attribute on a signal item.

Aneurysm9 avatar Aug 16 '22 15:08 Aneurysm9

As discussed in the spec SIG today I wonder whether it would be beneficial to define a requirement that scope attributes be prefixed with otel.scope when processed by non-OTel components. This would align with otel.scope.name and otel.scope.version and provide a clear indication that an attribute was a scope attribute and not an attribute on a signal item.

This results in a desirable outcome for short_name however I am not so sure it is desirable for other attributes. For example there is an ongoing discussion to use event.domain scope attribute. Prepending it with otel.scope will result in otel.scope.event.domain which is unnecessarily long and and harder to read. I think keeping event.domain as is would be better - it is shorter and meets the uniqueness requirements since we can make event. a semantic convention namespace.

tigrannajaryan avatar Aug 16 '22 17:08 tigrannajaryan

Is it to bad to suggest that only root-level attributes get a suffix (short_name becomes scope.short_name or otel.scope.short_name), as they are de facto considered meta attributes?

carlosalberto avatar Aug 17 '22 14:08 carlosalberto

Approving, but as mentioned in the Spec call too, let's allow some days for some days to iron out final details.

carlosalberto avatar Aug 17 '22 14:08 carlosalberto

(I could not attend the Spec SIG yesterday and the recording is not yet available.)

From the discussion above, looks like we're still debating what to call this attribute and whether it should have "otel.scope" as a prefix.

@dashpole asked two weeks ago:

Maybe something like name_prefix would be clearer that the intention is for this to be a name prefix

I suggest that namespace best describes the thing we're talking about. The Prometheus exporter would be specified to construct metric names by stitching the namespace, _, and the metric name together. This namespace concept that we are describing exactly matches OpenMetrics, which is nice.

I think otel.scope. should remain the suggested prefix for instrumentation scope name and version concepts, because they are OpenTelemetry concepts. I would not automatically apply a otel.scope. prefix to any of the scope attributes. (Tangent: My guess is that a standard prefix is intended to make it easier to flatten the attributes into a list. Question: why flatten in the first place, when OpenMetrics specifically describes the use of join??)

jmacd avatar Aug 17 '22 19:08 jmacd

Why flatten in the first place, when OpenMetrics specifically describes the use of join?

I don't think we should flatten for Prometheus/OpenMetrics. I've proposed a join-able info metric in https://github.com/open-telemetry/opentelemetry-specification/pull/2703. I think the guidance is for other non-OTLP exporters.

A scope is already somewhat of a namespace, and i'm hoping to introduce something thats closely tied to the scope name to avoid creating a new concept. I also worry an un-prefixed namespace would collide with metric/span/logRecord attributes often.

dashpole avatar Aug 18 '22 03:08 dashpole

I suggest that namespace best describes the thing we're talking about. The Prometheus exporter would be specified to construct metric names by stitching the namespace, _, and the metric name together. This namespace concept that we are describing exactly matches OpenMetrics, which is nice.

Is this Scope namespace concept only useful for metrics or we can generalize it and use uniformly for all signals?

tigrannajaryan avatar Aug 18 '22 14:08 tigrannajaryan

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

github-actions[bot] avatar Aug 26 '22 04:08 github-actions[bot]

Based on https://github.com/open-telemetry/opentelemetry-specification/pull/2703#issuecomment-1231892392, this isn't required anymore.

dashpole avatar Aug 31 '22 18:08 dashpole