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

Change timestamp data type to sfixed64 nanoseconds since Unix epoch #45

Conversation

tigrannajaryan
Copy link
Member

The change to int64 is proposed by RFC0059 [1]. In this commit there is a small deviation from
the RFC: here we use sfixed64 instead of int64 to make it consistent with changes already
done for Metrics proto [2]. Experiments proved sfixed64 to be more suitable for timestamps.

[1] https://github.com/open-telemetry/oteps/blob/master/text/0059-otlp-trace-data-format.md
[2] #33

The change to int64 is proposed by RFC0059 [1]. In this commit there is a small deviation from
the RFC: here we use sfixed64 instead of int64 to make it consistent with changes already
done for Metrics proto [2]. Experiments proved sfixed64 to be more suitable for timestamps.

[1] https://github.com/open-telemetry/oteps/blob/master/text/0059-otlp-trace-data-format.md
[2] open-telemetry#33
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/span_timestamp branch from fed3f83 to 3fc6412 Compare November 8, 2019 17:22
@yurishkuro
Copy link
Member

Experiments proved sfixed64 to be more suitable for timestamps.

more suitable than fixed64 too? signed value seems odd for the epoch-style timestamp.

@tigrannajaryan
Copy link
Member Author

Experiments proved sfixed64 to be more suitable for timestamps.

more suitable than fixed64 too? signed value seems odd for the epoch-style timestamp.

fixed64 should be slightly better (double the range in the future), but we already use sfixed64 in metrics. Fro consistency it is best to use the same type for timestamp everywhere. I can update in all places (not sure if we want it in this PR or separate).

@yurishkuro
Copy link
Member

If there's no perf benefit of sfixed64, I would go for fixed64, which is unsigned and makes more sense for epoch-style timestamps. Otherwise we'll be answering this question over and over.

@bogdandrutu
Copy link
Member

I agree we can change metrics too to use fixed64

@SergeyKanzhelev
Copy link
Member

merging this. @tigrannajaryan can you please send separate PR for switch to unsigned everywhere

@SergeyKanzhelev SergeyKanzhelev merged commit 920a0d0 into open-telemetry:master Nov 9, 2019
@tigrannajaryan tigrannajaryan deleted the feature/tigran/span_timestamp branch November 10, 2019 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants