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

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

Open
anuraaga opened this issue Dec 4, 2020 · 2 comments
Open

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

anuraaga opened this issue Dec 4, 2020 · 2 comments
Labels
area:semantic-conventions Related to semantic conventions priority:p2 Medium priority level release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:trace Related to the specification/trace directory

Comments

@anuraaga
Copy link
Contributor

anuraaga commented Dec 4, 2020

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.

@anuraaga anuraaga added the spec:trace Related to the specification/trace directory label Dec 4, 2020
@Oberon00
Copy link
Member

Oberon00 commented Dec 4, 2020

The related build-tools issue: open-telemetry/build-tools#13

@Oberon00
Copy link
Member

Oberon00 commented Dec 4, 2020

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.

@Oberon00 Oberon00 added the area:semantic-conventions Related to semantic conventions label Dec 7, 2020
@andrewhsu andrewhsu added priority:p2 Medium priority level release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs labels Dec 8, 2020
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 priority:p2 Medium priority level release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:trace Related to the specification/trace directory
Projects
None yet
Development

No branches or pull requests

3 participants