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

[Metrics SDK] Switch to explicit 64 bit integers #1639

Closed
sam-leitch-oxb opened this issue Sep 28, 2022 · 3 comments · Fixed by #1686
Closed

[Metrics SDK] Switch to explicit 64 bit integers #1639

sam-leitch-oxb opened this issue Sep 28, 2022 · 3 comments · Fixed by #1686
Assignees
Labels
priority:p1 Issues that are blocking

Comments

@sam-leitch-oxb
Copy link

sam-leitch-oxb commented Sep 28, 2022

All "Long" instruments in the API accept long as an input value. long does not have a well defined bit depth and can represent either 64 bit or 32 bit integers depending on the compiler implementation (and configuration).

"Long" instruments should be changed to accept int64_t integer values, which are unambiguously 64 bit depth as requested by the API specification: https://opentelemetry.io/docs/reference/specification/common/#attribute

@esigo esigo added this to the Metrics v1.0.0 (GA) milestone Sep 28, 2022
@sam-leitch-oxb sam-leitch-oxb changed the title Switch to explicit 64 bit integers [Metrics SDK] Switch to explicit 64 bit integers Sep 28, 2022
@lalitb
Copy link
Member

lalitb commented Sep 29, 2022

Specs don't say anything about the type/size of the measurement value. The above link is explicitly for attribute values. Ideally, we can support both fixed (32-bit, 64-bit) and architecture-dependent (long, int) data types at the API surface. At the SDK level, the generated metrics can be stored as a 64-bit integer.

@sam-leitch-oxb
Copy link
Author

You're right. My mistake. I can't find anything that explicitly requires specific value types.

My goal was only to prevent unintended integer overflow on 32 bit compilers, since counters that pass the 32bit boundary are not-uncommon (eg. timestamps, memory usage, network bytes read/written)

@lalitb
Copy link
Member

lalitb commented Sep 30, 2022

My goal was only to prevent unintended integer overflow on 32 bit compilers, since counters that pass the 32bit boundary are not-uncommon (eg. timestamps, memory usage, network bytes read/written)

Yes, this is a valid concern, we should fix this at the API level. Thanks for raising the issue.

@lalitb lalitb added the priority:p1 Issues that are blocking label Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:p1 Issues that are blocking
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants