Stats: Make defensive copy and set end time for ViewData when export. #512
Conversation
| @@ -145,3 +142,11 @@ def get_metrics(self, timestamp): | |||
| metric = metric_utils.view_data_to_metric(vd, timestamp) | |||
| if metric is not None: | |||
| yield metric | |||
|
|
|||
| # TODO(issue #470): remove this method once we export immutable stats. | |||
| def copy_view_data(self, view_data): | |||
There was a problem hiding this comment.
nit: Not sure how others feel about naming, but maybe something like finalize_view_data to make it clear that this also modifies it? Not super important given that this should go away soon.
There was a problem hiding this comment.
Actually this method does both - it first makes a copy of the original view_data and then sets the end_time of that copy. I renamed it to copy_and_finalize_view_data to make this clear.
| @@ -378,3 +378,20 @@ def test_export(self): | |||
| measure_to_view_map._registered_measures = {} | |||
| measure_to_view_map.export(view_data) | |||
| self.assertTrue(True) | |||
|
|
|||
| def test_export_duplicates_viewdata(self): | |||
There was a problem hiding this comment.
This test might be too close to just asserting the implementation to be useful, you may want to change it to e.g. check that the end timestamp is getting set correctly. Since this PR is a bugfix I think it ought to include a test to prevent a regression, up to you whether you think this does that.
There was a problem hiding this comment.
Thanks for adding the test, I've fixed a failure with mock and made it test if end_time is properly updated.
songy23
force-pushed
the
copy-view-data
branch
from
f582a14 to
f1ce0af
Compare
songy23
force-pushed
the
copy-view-data
branch
from
f1ce0af to
13f724b
Compare
|
Thanks for the review! |
Updates #509.