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

Vague spec: Span attributes may be "numeric" #425

Closed
Oberon00 opened this issue Jan 27, 2020 · 4 comments · Fixed by #722
Closed

Vague spec: Span attributes may be "numeric" #425

Oberon00 opened this issue Jan 27, 2020 · 4 comments · Fixed by #722
Assignees
Labels
area:api Cross language API specification issue priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:protocol Related to the specification/protocol directory spec:trace Related to the specification/trace directory
Milestone

Comments

@Oberon00
Copy link
Member

Oberon00 commented Jan 27, 2020

See https://github.com/open-telemetry/opentelemetry-specification/blame/2316771e7e0ca3bfe9b2286d13e3a41ded6b8858/specification/api-tracing.md#L366:

(Required) The attribute value, which is either:

  • A primitive type: string, boolean or numeric.
    ...

"Numeric" is very vague. Do we allow integers? How big? Floats? Which precision? Rationals? Decimals? Imaginary numbers? (Python even has literals for them).

I think it would be best to recommend what (not) to support. For example, the current opentelemetry-proto supports signed int64 and IEEE double.

@carlosalberto
Copy link
Contributor

The integer/float case is relatively simple, as it has been implicitly taken for granted, and we should cook a PR making this part clear.

Not sure on the size/precision side though. Should we define that based on the OTel protos? Guess not quite?

@carlosalberto carlosalberto added this to the Alpha v0.4 milestone Jan 30, 2020
@Oberon00
Copy link
Member Author

Oberon00 commented Jan 30, 2020

We could take a C++-standard-like approach here and guarantee only a minimum set that protocols should support, like "signed and at least 32 bit int" , "IEEE float or double".

@carlosalberto
Copy link
Contributor

guarantee only a minimum set that protocols should support

This should be good enough, I feel.

@jmacd
Copy link
Contributor

jmacd commented Feb 3, 2020

The state of the spec is here: https://github.com/open-telemetry/opentelemetry-proto/blob/master/opentelemetry/proto/common/v1/common.proto

It uses IEEE double and int64. I would be happy if complex numbers and larger numbers were encoded as strings.

@carlosalberto carlosalberto modified the milestones: v0.5, v0.6 Jun 9, 2020
@bogdandrutu bogdandrutu self-assigned this Jun 12, 2020
@carlosalberto carlosalberto added spec:trace Related to the specification/trace directory area:api Cross language API specification issue spec:protocol Related to the specification/protocol directory release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Jun 26, 2020
@andrewhsu andrewhsu added the priority:p1 Highest priority level label Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue priority:p1 Highest priority level release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:protocol Related to the specification/protocol directory spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants