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

Clarify that 64 bit integer numbers are decimal strings in OTLP/JSON #1637

Merged

Conversation

tigrannajaryan
Copy link
Member

Resolves open-telemetry/opentelemetry-proto#268

This behavior is already the documented behavior for JSON Protobuf,
see https://developers.google.com/protocol-buffers/docs/proto3#json:

JSON value will be a decimal string. Either numbers or strings are accepted.

Resolves open-telemetry/opentelemetry-proto#268

This behavior is already the documented behavior for JSON Protobuf,
see https://developers.google.com/protocol-buffers/docs/proto3#json:

>JSON value will be a decimal string. Either numbers or strings are accepted.
@tigrannajaryan tigrannajaryan requested review from a team as code owners April 22, 2021 18:17
@tigrannajaryan tigrannajaryan added the spec:protocol Related to the specification/protocol directory label Apr 22, 2021
@obecny
Copy link
Member

obecny commented Apr 22, 2021

In collector exporter the number will be sent as intValue for numbers between -2147483648 to 2147483647;
Other numbers will be sent as doubleValue
https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-exporter-collector/src/transform.ts#L93

When we send data as proto then the whole data is being converted using protobufjs package which reads the proto file definitions and converts it automatically.

@tigrannajaryan
Copy link
Member Author

tigrannajaryan commented Apr 23, 2021

In collector exporter the number will be sent as intValue for numbers between -2147483648 to 2147483647;
Other numbers will be sent as doubleValue
https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-exporter-collector/src/transform.ts#L93

@obecny Neither int32 nor double are good enough for timestamps (which is one of the most common uses cases for 64 bit integers in proto). int32 does not have enough range, double does not have enough precision. I do not know what are the options to deal with 64 bit integer numbers in JS and I understand it is painful but unfortunately this is the current reality of the proto.

This PR does not change that at all. This is just a clarification of how 64 bit integer numbers (which we have plenty in OTLP) work in Protobuf JSON encoding.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@tigrannajaryan tigrannajaryan merged commit b2375d7 into open-telemetry:main Apr 27, 2021
@tigrannajaryan tigrannajaryan deleted the feature/tigran/otlp-json branch April 27, 2021 16:46
tigrannajaryan added a commit that referenced this pull request Oct 13, 2021
The data model says that timestamps are nanosecond values, but examples use milliseconds.
Updated to use stringified nanoseconds according to #1637

Fixes: #1049
tigrannajaryan added a commit to tigrannajaryan/opentelemetry-specification that referenced this pull request Oct 15, 2021
The data model says that timestamps are nanosecond values, but examples use milliseconds.
Updated to use stringified nanoseconds according to open-telemetry#1637

Fixes: open-telemetry#1049
tigrannajaryan added a commit to tigrannajaryan/opentelemetry-specification that referenced this pull request Oct 15, 2021
The data model says that timestamps are nanosecond values, but examples use milliseconds.
Updated to use stringified nanoseconds according to open-telemetry#1637

Fixes: open-telemetry#1049
SergeyKanzhelev added a commit that referenced this pull request Oct 15, 2021
The data model says that timestamps are nanosecond values, but examples use milliseconds.
Updated to use stringified nanoseconds according to #1637

Fixes: #1049

Co-authored-by: Sergey Kanzhelev <S.Kanzhelev@live.com>
joaopgrassi pushed a commit to dynatrace-oss-contrib/semantic-conventions that referenced this pull request Mar 21, 2024
The data model says that timestamps are nanosecond values, but examples use milliseconds.
Updated to use stringified nanoseconds according to open-telemetry/opentelemetry-specification#1637

Fixes: open-telemetry/opentelemetry-specification#1049

Co-authored-by: Sergey Kanzhelev <S.Kanzhelev@live.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:protocol Related to the specification/protocol directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decide how to handle 64 bit integers in JSON encoding
7 participants