make BucketedTimeSeries::addValue() honor old timestamps
authorAdam Simpkins <simpkins@fb.com>
Wed, 14 May 2014 20:40:33 +0000 (13:40 -0700)
committerAnton Likhtarov <alikhtarov@fb.com>
Mon, 9 Jun 2014 22:33:57 +0000 (15:33 -0700)
commit253dd087cc83e0b921b73d63477a238fb707f28b
treecc2bfb0eb3262727d07018e9475b29201a0b566b
parent527ce886369194ebbdaef842a0d96445203d171c
make BucketedTimeSeries::addValue() honor old timestamps

Summary:
Previously BucketedTimeSeries()::addValue() documented that it required
time to move forwards.  If it was ever called with a timestamp older
than the most recent one it had seen, it would just use latestTime_ as
the time, and add the value to the most recent bucket.

This changes addValue() so that it always uses the timestamp passed in
by the caller.  If this time value refers to an old bucket that is still
being tracked, the data point will be added to that bucket.  If the time
value is older than the oldest currently tracked bucket, the data point
will be ignored, and false will be returned.

I did consider leaving the current addValue() behavior as-is, and
requiring a separate addHistoricalValue() for when users intentionally
want to try adding old data points.  However, it seems nicer to build
this into the existing addValue() function.  The old behavior of just
replacing the supplied time value seems potentially surprising to users.

This does change the behavior of addValue(), and therefore could affect
the behavior of some programs.  However, up until now no-one should have
been calling addValue() with old time values, as it wouldn't have done
what they want anyway.  I did a brief search through our code base, and
all call sites I saw always called addValue() with the current time.
(Most of the callers use wall clock time, so this change might affect
program behavior if the system time changes after the program starts.
We should ideally change our programs to use monotonic clocks instead.)

Test Plan:
Included a new unit test.

Also compared the timeseries_benchmark results before and after this
change.  Overall this new logic seems to be faster.  For the "all time"
case, the new code is over 2x as fast.  For the normal, non-all-time
case the new code is around 5% faster.

Reviewed By: hans@fb.com

Subscribers: doug, folly@lists, net-systems@, exa

FB internal diff: D1338466
folly/stats/BucketedTimeSeries-defs.h
folly/stats/BucketedTimeSeries.h
folly/test/TimeseriesTest.cpp