open-telemetry / opentelemetry-specification

Specifications for OpenTelemetry
https://opentelemetry.io
Apache License 2.0
3.72k stars 888 forks source link

The `direction` attribute does not comply with the naming guidelines in the spec #3129

Closed sebastien-rosset closed 5 months ago

sebastien-rosset commented 1 year ago

What are you trying to achieve?

Either:

  1. Fix the name of the direction attribute so that it's compliant with the naming guidelines.
  2. Amend the naming guidelines in the spec to handle scenarios such as direction.

What did you expect to see?

I was expected the OpenTelemetry spec to be self-consistent with naming conventions, in particular that the direction name is hierarchical.

Additional context.

The spec states the instrument should have attributes for direction; oddly, plural is used, which implies there could be more than one attribute for direction, but without explaining why plural makes sense. Further, that sentence is not stating the attribute should be named direction, but elsewhere it was interpreted as a literal requirement, i.e., the name of this attribute IS direction.

io - an instrument that measures bidirectional data flow should be called entity.io and have attributes for direction. For example, system.network.io.

Using the name direction breaks some of the semantic rules listed in the spec:

  1. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/README.md#general-guidelines and
  2. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/common/attribute-naming.md#attribute-naming

Metric names and attributes exist within a single universe and a single hierarchy. Metric names and attributes MUST be considered within the universe of all existing metric names. When defining new metric names and attributes, consider the prior art of existing standard metrics and metrics from frameworks/libraries.

One issue is that another namespace could have a different concept of "direction" such as up, down, or East, West, left, right, etc. For example a vane anemometer could have a direction attribute which reports wind direction in degrees or cardinal direction. Perhaps for I/O the attribute should be named hw.io_direction.

In addition, the spec states:

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.

Defining attributes with no name hierarchy has the side effect of preventing any future use of that word as a namespace.

bertysentry commented 1 year ago

Semantic conventions for System, Runtime Environment and Process use these attributes without prefixes:

direction is notably used for the following metrics:

Semantic conventions for hardware simply follow the same pattern and ensure consistency for this attribute and its possible values, as recommended by the general guidelines for metrics semconv.

My advice is that we must update the specification for attributes naming. While it is critical to have unique attribute names for Resources, it should not be mandatory for metric attributes.

Metric attributes are defined in the scope of a given metric (in OpenTelemetry) and there is no possible conflict of 2 attributes with identical names from 2 different metrics.

Having direction attribute in process.disk.io, system.network.io and hw.network.io is never going to be a problem.

If we can avoid prefixing everything as if it were Java class names, it will make everybody's life much easier too, and save some storage space as well ;-)

sebastien-rosset commented 1 year ago

Thanks. If that's the case, then yes the specification for attributes naming should be updated. This would apply to #3131 as well.

Given that the spec explicitly states attribute names must be hierarchical, one could wrongly assume metric attributes do not have name collisions across metrics. This could be a problem when exporting metrics and their attributes to an external database. The spec could explicitly state the names of metric attributes are unique within the scope of a metric.

sebastien-rosset commented 1 year ago

Presumably the spec would also have to state something about naming conventions for attributes defined in the spec itself versus attributes defined by other organizations. Someone could create metric-level attributes with common words or contractions such as com, memory, mtu, buffer, etc, which may collide with future use of the same words in the spec but with different semantics.

sebastien-rosset commented 1 year ago

Thank you for the clarification.

Metric attributes are defined in the scope of a given metric (in OpenTelemetry) and there is no possible conflict of 2 attributes with identical names from 2 different metrics.

I thought the specification allowed for extensibility? I.e., a per-metric attribute can be defined in the specification, and a vendor may also define custom attributes. At least this is how I understood the recommendation to use hierarchical names. A hierarchical name such as com.myCompany.foo.bar helps to avoid collisions, even if the spec adds new attributes in future versions of the spec. If a vendor can create a per-metric attribute with a flat name, then I don't see how it's possible to avoid future collisions. For example, one may want to create custom attributes such as form_factor or port_type. In that case, maybe the vendor will be lucky, or maybe not.

Shouldn't there be a way to ensure there is no possible collision between custom attributes and standard attributes?

dmitryax commented 8 months ago

Looks like this issue is resolved now. direction was recently changed to have different prefixes depending on a use case: system.paging.direction, disk.io.direction, network.io.direction.

svrnm commented 5 months ago

We feel that this has been addressed, if you feel differently, leave a comment and we can reopen the issue, @sebastien-rosset