search

open-telemetry / opentelemetry-specification

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

Attribute identities on metric streams #2143

Open legendecas opened 2 years ago

legendecas commented 2 years ago

What are you trying to achieve?

As the state, Attribute values can be primitive values like string, boolean, double and int64 (https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/common/common.md#attribute). SDKs treat Attribute as part of the metric stream identity (https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/datamodel.md#opentelemetry-protocol-data-model). So this seems that an attribute with value "0", false, 0.0, and 0 (and possibly null for reasons below) should be identified as different identities, i.e. there should be 4 (or 5) unique metric streams in memory for each of these values respectively.

However, there are two problems with this behavior:

  1. exporters that do not support null value on export can coarse the null to 0, false, or empty strings "". (stated in https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/common/common.md#attribute)
  2. exporters, that only support string value attribute values (like Prometheus), have to coarse all values to strings. Like, both 0 and "0" are exported as "0".

So on exporting, several unique metric streams in SDK can be exported as indistinguishable two streams. I'm not sure if this is designed as is, but it seems at first look not suitable in cases.

Possible solutions

  1. Limit the metric attribute values to be string values.
  2. Clarify on the identity issues and leave it to users to decide if using values other than strings.
reyang commented 2 years ago

OTLP streams will keep the type. Other streams have freedom to define their behavior. OTLP (preserves types) to Prometheus (string only) will have lossy conversion.

@jsuereth has an old PR on the Prometheus data model. Supporting boolean, number and non-empty string should be easy; null, empty string are tricky - should we prevent these from the API?

@reyang in the PrometheusExporter prototype for .NET - null will be converted to "(null)" for systems that only supports text. Empty string label (in PrometheusExporter) will be dropped (to be consistent with Prometheus).

@dyladan JavaScript has undefined, null and unset which make things harder. Unset if not a problem because it can be treated as the attribute key-value pair does not exist. null in JavaScript means the user explicitly gives the value that is intentionally set as null, undefined by convention means it's not intentional.

For JavaScript, update the document to clarify that null and undefined have undefined behavior in the API, it's up to each exporter how to handle it.

The Prometheus related specs are still work-in-progress, but it is very likely that the following conversion would happen:

legendecas commented 2 years ago

Not every language stringifies true and false with "true" and "false". It is "True" and "False" in python. Should the serialized value to be language specific?

jsuereth commented 2 years ago

@legendecas for now, i'd stick with what the collector does here for Prometheus. It's likely we'll specify what the collector does as the common denominator between implementations. If this causes issues we can prioritize specification here.