search

open-telemetry / opentelemetry-specification

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

Semantic conventions should use int or double, not number #1274

Open anuraaga opened 4 years ago

anuraaga commented 4 years ago

What are you trying to achieve?

Easy mapping from semantic conventions to OTLP / sane handling in OpenTelemetry collector.

Additional context.

In the collector, we have logic to parse HTTP status code

https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/master/exporter/awsxrayexporter/translator/http.go#L58

It calls IntVal() which is the pdata, similar to OTLP, representation of data in the collector. But since the semantic convention only specifies number

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/http.md#common-attributes

It seems like currently, the only way to properly parse a number is to check both intval and doubleval. Is this intended? I guess the semantic convention types are supposed to match the OTLP types. This came up since currently JS populates all numeric attributes as double - this is probably because JS uses double for all numbers. This seems wrong, but with the spec as is, it technically is ok.

Oberon00 commented 4 years ago

The related build-tools issue: https://github.com/open-telemetry/build-tools/issues/13

Oberon00 commented 4 years ago

It seems like currently, the only way to properly parse a number is to check both intval and doubleval. Is this intended? I guess the semantic convention types are supposed to match the OTLP types.

No, the semantic convention types are not bound by OTLP. Rather they should correspond to what is allowed in the specification for https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/common/common.md#attributes. Since OTLP is modeled after the same source, this is a non-issue today, but when the generator tool was written, #425 was not resolved and we only that the type "numeric" in the spec, not int64/double.

This is the historical explanation for how we ended up only having "number", I agree this should be improved to distinguish int64/double.