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

fix(otel_metrics): use opentelemetry:timestamp/0 #644

Merged
merged 1 commit into from
Oct 28, 2023

Conversation

SergeTupchiy
Copy link
Contributor

There was inconsistency in timestamps handling: metrics used system_time(nanosecond), while metrics exporter treated these values as opentelemetry:timestamp() (monotonic time in native units).

This PR fixes it by using opentelemetry:timestamp(), most of the diff is caused by renaming *time_unix_nano to time to avoid confusions.

…onic time)

Timestamps are converted to Unix nano timestamps by the exporter.
@SergeTupchiy SergeTupchiy requested a review from a team October 24, 2023 12:33
@SergeTupchiy
Copy link
Contributor Author

@tsloughter , @bryannaegele, @ferd
Could you please take a look at this PR?
Alternative fix would be to avoid converting timestamps in otel_otlp_metrics: https://github.com/open-telemetry/opentelemetry-erlang/blob/main/apps/opentelemetry_experimental/src/otel_otlp_metrics.erl#L89
But I think the proposed fix is better as it follows the same approach used for traces: https://github.com/open-telemetry/opentelemetry-erlang/blob/main/apps/opentelemetry_api/src/opentelemetry.erl#L305

@bryannaegele
Copy link
Contributor

This seems good? I haven't been involved in the metrics but this seems cleaner and more consistent.

@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

see 55 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@tsloughter
Copy link
Member

Thanks, yea, I think this looks right.

The only thing I'm debating, which can be a separate issue/pr, is should we really be using monotonic_time instead of just system_time... I don't think we ever use opentelemetry:timestamp anywhere it gets used to compare or find a diff. Maybe safest to allow it to be used for comparisons/diffs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants