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

Remove timestamp read per metric update #2137

Merged
merged 2 commits into from
Jul 15, 2021

Conversation

cijothomas
Copy link
Member

Changes

Removes the usage of DateTimeOffset in every measurement record event. There seems to be no need of paying cost for reading/passingaround/storing the time with every Update call.

The StartTime is read when the Aggregators start up. (same as before)
The EndTime is ready when "collect" occurs. (same as before)

There can be some more changes done the same area (in future PRs), where we read system time just once at startup, and then rely purely on tick counts, to calculate endtimes for each Collect/Export cycle.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@cijothomas cijothomas requested review from a team and victlu July 15, 2021 14:28
Copy link
Contributor

@victlu victlu left a comment

Choose a reason for hiding this comment

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

It feels funny to not have DT per measurement, but technically, we can leave this to Aggregators that actually need it.

@cijothomas
Copy link
Member Author

It feels funny to not have DT per measurement, but technically, we can leave this to Aggregators that actually need it.

I cannot find a reason for knowing the timestamp at which the recording was made, as the output is an aggregation between a start and end time.
We'll need it in Exemplars(https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/metrics/v1/metrics.proto#L528), and can be added if that feature is on, and as you said, it can be inside the individual aggregator itself.

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.

@cijothomas cijothomas merged commit 9a5d9fd into metrics Jul 15, 2021
@cijothomas cijothomas deleted the cijothomas/metric_removetimestampperupdat branch July 15, 2021 16:33
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.

3 participants