Stats: Timestamps not updated when new measurement is recorded

[colincadams]

A call to MeasurementMap.record creates a timestamp and then passes that through MeasureToViewMap.record -> ViewData.record -> AggregationData.add_sample but is never persisted anywhere.

This means that the timestamp of when the view is registered is used whenever measurements are exported. For stackdriver this means that the first time the time series are sent from a particular instance it succeeds, but any subsequent call fails because the timestamp is too close to the prior timestamp or if the instance has been up for more than a day it fails as the timestamp is too old.

I would expect the _end_time of ViewData to be updated when a new measurement is recorded, which would fix this problem for cumulative metrics. Depending on how _start_time and _end_time are used when exporting, this may be the correct solution for gauges as well.

[songy23]

end_time is set when ViewData gets exported:

opencensus-python/opencensus/stats/measure_to_view_map.py

Line 72 in f669aa9

view_data_copy.end()

. So end_time will always be newer than start_time.

We don't use the timestamp associated with measurements except in Exemplars, since ViewData represents aggregated data. During aggregation each individual measurement and timestamp are dropped.

[colincadams]

get_view doesn't seem to be called on export. Maybe it should be? Instead in record the items in the underlying map are just iterated over, a list is created, and that is passed to export.

opencensus-python/opencensus/stats/measure_to_view_map.py

Line 129 in f669aa9

e.export(view_datas)

I don't completely understand why it being aggregated data means the timestamp shouldn't be used. To me the aggregated data represents the data for the interval up to and including the timestamp passed in, so the start and end should reflect that. Having end_time set there instead of mutating it in the getter seems less error prone.

Otherwise maybe end_time should always be None for the ones in the map, and only set on export? Then maybe the errors would be more obvious? That begs the question as to why ViewData even has start and end times however. If they are simply going to be set on export, and a ViewData itself doesn't have a fixed interval then I would argue it shouldn't have timestamps and the timestamp should just be an additional argument to export on the MeasureToViewMap.

What do you think?

[songy23]

get_view doesn't seem to be called on export. Maybe it should be? Instead in record the items in the underlying map are just iterated over, a list is created, and that is passed to export.

Yes in other languages we do in this way. Even if get_view is not called in export, we should at least call view_data.end() at the time when exporting. This is a bug that we need to fix.

I don't completely understand why it being aggregated data means the timestamp shouldn't be used. To me the aggregated data represents the data for the interval up to and including the timestamp passed in, so the start and end should reflect that. Having end_time set there instead of mutating it in the getter seems less error prone.

There're several reasons that I can think of. One is to avoid overhead when recording. Our expectation is that recording measurements will be much more frequent than exporting so we would like to avoid resetting the timestamp whenever a new measurement go in. The other reason is we treat ViewData as aggregated data over a fixed time window. When exporting ViewDatas we want all of them have the same aggregation window, otherwise we may need to sort the ViewDatas by end_time because some backend requires strict time ordering.

Otherwise maybe end_time should always be None for the ones in the map, and only set on export? Then maybe the errors would be more obvious? That begs the question as to why ViewData even has start and end times however. If they are simply going to be set on export, and a ViewData itself doesn't have a fixed interval then I would argue it shouldn't have timestamps and the timestamp should just be an additional argument to export on the MeasureToViewMap.

I kind of agree with your opinion. In other languages ViewData represents an immutable snapshot of current aggregated data. The underlying aggregation (we call it MutableViewData in Java) keeps changing, and every time when exporting we grab a snapshot (ViewData) of the mutable aggregated data and set the start and end time. MutableViewData doesn't have a notion on end_time.

In Python unfortunately we didn't make clear separation between the mutable and immutable aggregated data, and ViewData is used in both scenarios. There's a tracking issue to make this consistent with other languages (#470).

[colincadams]

Gotcha, the MutableViewData and ViewData distinction makes a lot of sense to me, glad to see there is an open issue for that.

Do you think it would make sense to handle this bug first, making view data copies and calling end in export (or just calling get_view) and then do #470 separately? Or will #470 be addressed soon?

[songy23]

Yes I'm working on a quick fix for this issue first.

[songy23]

The immediate fix (#512) is submitted, and the follow-up changes are covered by #470 and #454. Going to mark this issue as fixed for now, please feel free to reopen if the problem persists.