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.

Sign up for GitHub

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
merged 2 commits into from
Dec 2, 2024
Merged

Time is not optional in DataPoints #2367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
77a99d7
Time is not optional in DataPoints
fraillt Nov 30, 2024
127164f
Merge branch 'main' into time-not-optional-for-data-points
cijothomas Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions opentelemetry-proto/src/transform/metrics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,8 +295,8 @@
.iter()
.map(|dp| TonicNumberDataPoint {
attributes: dp.attributes.iter().map(Into::into).collect(),
start_time_unix_nano: dp.start_time.map(to_nanos).unwrap_or_default(),
time_unix_nano: dp.time.map(to_nanos).unwrap_or_default(),
start_time_unix_nano: to_nanos(dp.start_time),
time_unix_nano: to_nanos(dp.time),

Check warning on line 299 in opentelemetry-proto/src/transform/metrics.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-proto/src/transform/metrics.rs#L298-L299 

Added lines #L298 - L299 were not covered by tests
exemplars: dp.exemplars.iter().map(Into::into).collect(),
flags: TonicDataPointFlags::default() as u32,
value: Some(dp.value.into()),
Expand All @@ -319,8 +319,8 @@
.iter()
.map(|dp| TonicNumberDataPoint {
attributes: dp.attributes.iter().map(Into::into).collect(),
start_time_unix_nano: dp.start_time.map(to_nanos).unwrap_or_default(),
time_unix_nano: dp.time.map(to_nanos).unwrap_or_default(),
start_time_unix_nano: to_nanos(dp.start_time),
time_unix_nano: to_nanos(dp.time),

Check warning on line 323 in opentelemetry-proto/src/transform/metrics.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-proto/src/transform/metrics.rs#L322-L323 

Added lines #L322 - L323 were not covered by tests
exemplars: dp.exemplars.iter().map(Into::into).collect(),
flags: TonicDataPointFlags::default() as u32,
value: Some(dp.value.into()),
Expand Down
8 changes: 4 additions & 4 deletions opentelemetry-sdk/src/metrics/data/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

/// The time when the time series was recorded.
pub time: Option<SystemTime>,
pub time: SystemTime,
/// The value of this data point.
pub value: T,
/// The sampled [Exemplar]s collected during the time series.
Expand Down Expand Up @@ -338,8 +338,8 @@ mod tests {
fn validate_cloning_data_points() {
let data_type = DataPoint {
attributes: vec![KeyValue::new("key", "value")],
start_time: Some(std::time::SystemTime::now()),
time: Some(std::time::SystemTime::now()),
start_time: std::time::SystemTime::now(),
time: std::time::SystemTime::now(),
value: 0u32,
exemplars: vec![Exemplar {
filtered_attributes: vec![],
Expand Down
20 changes: 10 additions & 10 deletions opentelemetry-sdk/src/metrics/internal/aggregate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,8 +224,8 @@ mod tests {
let mut a = Gauge {
data_points: vec![DataPoint {
attributes: vec![KeyValue::new("a", 1)],
start_time: Some(SystemTime::now()),
time: Some(SystemTime::now()),
start_time: SystemTime::now(),
time: SystemTime::now(),
value: 1u64,
exemplars: vec![],
}],
Expand All @@ -251,15 +251,15 @@ mod tests {
data_points: vec![
DataPoint {
attributes: vec![KeyValue::new("a1", 1)],
start_time: Some(SystemTime::now()),
time: Some(SystemTime::now()),
start_time: SystemTime::now(),
time: SystemTime::now(),
value: 1u64,
exemplars: vec![],
},
DataPoint {
attributes: vec![KeyValue::new("a2", 1)],
start_time: Some(SystemTime::now()),
time: Some(SystemTime::now()),
start_time: SystemTime::now(),
time: SystemTime::now(),
value: 2u64,
exemplars: vec![],
},
Expand Down Expand Up @@ -294,15 +294,15 @@ mod tests {
data_points: vec![
DataPoint {
attributes: vec![KeyValue::new("a1", 1)],
start_time: Some(SystemTime::now()),
time: Some(SystemTime::now()),
start_time: SystemTime::now(),
time: SystemTime::now(),
value: 1u64,
exemplars: vec![],
},
DataPoint {
attributes: vec![KeyValue::new("a2", 1)],
start_time: Some(SystemTime::now()),
time: Some(SystemTime::now()),
start_time: SystemTime::now(),
time: SystemTime::now(),
value: 2u64,
exemplars: vec![],
},
Expand Down
8 changes: 4 additions & 4 deletions opentelemetry-sdk/src/metrics/internal/last_value.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ impl<T: Number> LastValue<T> {
self.value_map
.collect_and_reset(dest, |attributes, aggr| DataPoint {
attributes,
start_time: Some(prev_start),
time: Some(t),
start_time: prev_start,
time: t,
value: aggr.value.get_value(),
exemplars: vec![],
});
Expand All @@ -79,8 +79,8 @@ impl<T: Number> LastValue<T> {
self.value_map
.collect_readonly(dest, |attributes, aggr| DataPoint {
attributes,
start_time: Some(prev_start),
time: Some(t),
start_time: prev_start,
time: t,
value: aggr.value.get_value(),
exemplars: vec![],
});
Expand Down
8 changes: 4 additions & 4 deletions opentelemetry-sdk/src/metrics/internal/precomputed_sum.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ impl<T: Number> PrecomputedSum<T> {
let delta = value - *reported.get(&attributes).unwrap_or(&T::default());
DataPoint {
attributes,
start_time: Some(prev_start),
time: Some(t),
start_time: prev_start,
time: t,
value: delta,
exemplars: vec![],
}
Expand Down Expand Up @@ -109,8 +109,8 @@ impl<T: Number> PrecomputedSum<T> {
self.value_map
.collect_readonly(&mut s_data.data_points, |attributes, aggr| DataPoint {
attributes,
start_time: Some(prev_start),
time: Some(t),
start_time: prev_start,
time: t,
value: aggr.value.get_value(),
exemplars: vec![],
});
Expand Down
8 changes: 4 additions & 4 deletions opentelemetry-sdk/src/metrics/internal/sum.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ impl<T: Number> Sum<T> {
self.value_map
.collect_and_reset(&mut s_data.data_points, |attributes, aggr| DataPoint {
attributes,
start_time: Some(prev_start),
time: Some(t),
start_time: prev_start,
time: t,
value: aggr.value.get_value(),
exemplars: vec![],
});
Expand Down Expand Up @@ -132,8 +132,8 @@ impl<T: Number> Sum<T> {
self.value_map
.collect_readonly(&mut s_data.data_points, |attributes, aggr| DataPoint {
attributes,
start_time: Some(prev_start),
time: Some(t),
start_time: prev_start,
time: t,
value: aggr.value.get_value(),
exemplars: vec![],
});
Expand Down
24 changes: 10 additions & 14 deletions opentelemetry-stdout/src/metrics/exporter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,20 +163,16 @@
fn print_data_points<T: Debug>(data_points: &[data::DataPoint<T>]) {
for (i, data_point) in data_points.iter().enumerate() {
println!("\t\tDataPoint #{}", i);
if let Some(start_time) = data_point.start_time {
let datetime: DateTime<Utc> = start_time.into();
println!(
"\t\t\tStartTime : {}",
datetime.format("%Y-%m-%d %H:%M:%S%.6f")
);
}
if let Some(end_time) = data_point.time {
let datetime: DateTime<Utc> = end_time.into();
println!(
"\t\t\tEndTime : {}",
datetime.format("%Y-%m-%d %H:%M:%S%.6f")
);
}
let datetime: DateTime<Utc> = data_point.start_time.into();
println!(
"\t\t\tStartTime : {}",
datetime.format("%Y-%m-%d %H:%M:%S%.6f")
);
let datetime: DateTime<Utc> = data_point.time.into();
println!(
"\t\t\tEndTime : {}",
datetime.format("%Y-%m-%d %H:%M:%S%.6f")
);

Check warning on line 175 in opentelemetry-stdout/src/metrics/exporter.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-stdout/src/metrics/exporter.rs#L166-L175 

Added lines #L166 - L175 were not covered by tests
println!("\t\t\tValue : {:#?}", data_point.value);
println!("\t\t\tAttributes :");
for kv in data_point.attributes.iter() {
Expand Down
Loading