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

Time is not optional in DataPoints #2367

Merged

Conversation

fraillt
Copy link
Contributor

@fraillt fraillt commented Nov 30, 2024

Changes

Make start_time and time non optional in DataPoint<T> (used for Sum and Gauge).

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@fraillt fraillt requested a review from a team as a code owner November 30, 2024 20:57
Copy link

codecov bot commented Nov 30, 2024

Codecov Report

Attention: Patch coverage is 63.15789% with 14 lines in your changes missing coverage. Please review.

Project coverage is 79.5%. Comparing base (6b71301) to head (127164f).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
opentelemetry-stdout/src/metrics/exporter.rs 0.0% 10 Missing ⚠️
opentelemetry-proto/src/transform/metrics.rs 0.0% 4 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main   #2367   +/-   ##
=====================================
  Coverage   79.5%   79.5%           
=====================================
  Files        123     123           
  Lines      21492   21488    -4     
=====================================
  Hits       17096   17096           
+ Misses      4396    4392    -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -97,9 +97,9 @@ pub struct DataPoint<T> {
/// time series.
pub attributes: Vec<KeyValue>,
/// The time when the time series was started.
pub start_time: Option<SystemTime>,
pub start_time: SystemTime,
Copy link
Member

Choose a reason for hiding this comment

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

It'd be good to mention in changelog as this is breaking anyone writing own exporters. I expect a lot more changes in this area, so it is probably better to club them all into one changelog entry.

Copy link
Contributor

Choose a reason for hiding this comment

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

Start time is marked optional by spec.

@cijothomas Currently, the SDK would always populate it, but do you think there could be a scenario where we wouldn't populate the start time. With MetricProducer or something else?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, producer might not provide it. so keeping Option for startTime might be safer then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

Copy link
Member

@cijothomas cijothomas Dec 2, 2024

Choose a reason for hiding this comment

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

@fraillt Could you keep Option<startTime> ? Sorry I merged this already, so needs to be a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was reading https://opentelemetry.io/docs/specs/otel/metrics/data-model/ and got impression that these fields can be in fact not optional...
Basically everything is required, except for Gauge one field:

(optional) A timestamp (start_time_unix_nano) which best represents the first possible moment a measurement could be recorded. This is commonly set to the timestamp when a metric collection system started.

In the proto definition same field has comment (emphasis is mine):

// StartTimeUnixNano is optional but strongly encouraged, see the detailed comments above Metric.

Maybe I'm reading outdated/invalid documentation?

Copy link
Member

Choose a reason for hiding this comment

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

(optional) A timestamp (start_time_unix_nano) which best represents the firs

^ This part is where startTime is mentioned to be optional. An external system interfacing via Producer to OTel may not have startTime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just want to understand better... at the moment it's not possible that Gauge will have None using SdkMeterProvider.
So basically, if you want to support Gauge with optional start time, at minimum you would have to implement your own, MeterProvider, InstrumentProvider, and your own SyncInstrument. Which also means that you need to implement your own collect implementation as well.
I understand that this is part of api and user should be able to do so. I'm wondering if you know a single example where anyone actually does that ? (implement core functionality itself' instead of using SdkMeterProvider).
Or in other words, do you have any example that SdkMeterProvider is not suitable and users write their own implementation of metrics ? (just curious)

Copy link
Member

Choose a reason for hiding this comment

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

When we start supporting Producers concept, where users aggregate metrics elsewhere and bridge it to OTel, startTime can be missing.
The current sdk has no scenario for startTime being optional, but we just want to make sure our public API remains stable even when adding Producer support.

Copy link
Member

@cijothomas cijothomas 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 5b6e9b9 into open-telemetry:main Dec 2, 2024
20 of 23 checks passed
pitoniak32 pushed a commit to pitoniak32/opentelemetry-rust that referenced this pull request Dec 4, 2024
@fraillt fraillt deleted the time-not-optional-for-data-points branch December 11, 2024 06:52
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