diff options
author | Arne Juul <arnej@yahoo-inc.com> | 2017-11-28 07:36:32 +0000 |
---|---|---|
committer | Arne Juul <arnej@yahoo-inc.com> | 2017-12-03 20:05:01 +0000 |
commit | 746556112b390e7f428b5c8c51353fcffd914f64 (patch) | |
tree | 8f90720e8c8c2c10917320f4759caf36b3f1fbe1 | |
parent | 2634665462f58cb5219ffe226b7f9e192350e0af (diff) |
review follow-up
* bug fix PointMap::operator< and use "for" loop
* construct aggregators from samples only
* use generation counter instead of timestamps to ensure correct ordering
* add "noexcept" to signal strong exception guarantees
* handle out-of-order metric type checks
* ensure ticker thread is stopped in destructor
* change TickerThread::stop() to only do its work once, as
calling stop() twice crashed for some strange reason
* refactor CurrentSamples extraction
* move time-handling to tick()
* ensure we do not have concurrent tick()s running
* simpler time handling
29 files changed, 272 insertions, 182 deletions
diff --git a/staging_vespalib/src/tests/metrics/simple_metrics_test.cpp b/staging_vespalib/src/tests/metrics/simple_metrics_test.cpp index 1bd10d90384..d0a7ca4c4a9 100644 --- a/staging_vespalib/src/tests/metrics/simple_metrics_test.cpp +++ b/staging_vespalib/src/tests/metrics/simple_metrics_test.cpp @@ -12,40 +12,55 @@ using namespace vespalib::metrics; TEST("require that simple metrics gauge merge works") { - MetricIdentifier id(MetricName(42)); - GaugeAggregator a(id), b(id), c(id); - b.observedCount = 3; - b.sumValue = 24.0; - b.minValue = 7.0; - b.maxValue = 9.0; - b.lastValue = 8.0; - - EXPECT_EQUAL(a.observedCount, 0u); + MetricIdentifier id(MetricName(42), Point(17)); + Gauge::Measurement a1(id, 0.0); + Gauge::Measurement b1(id, 7.0); + Gauge::Measurement b2(id, 9.0); + Gauge::Measurement b3(id, 8.0); + Gauge::Measurement c1(id, 10.0); + Gauge::Measurement c2(id, 1.0); + + GaugeAggregator a(a1), b(b1), c(c1); + b.merge(b2); + b.merge(b3); + c.merge(c2); + + EXPECT_EQUAL(a.observedCount, 1u); EXPECT_EQUAL(a.sumValue, 0.0); EXPECT_EQUAL(a.minValue, 0.0); EXPECT_EQUAL(a.maxValue, 0.0); EXPECT_EQUAL(a.lastValue, 0.0); + + EXPECT_EQUAL(b.observedCount, 3u); + EXPECT_EQUAL(b.sumValue, 24.0); + EXPECT_EQUAL(b.minValue, 7.0); + EXPECT_EQUAL(b.maxValue, 9.0); + EXPECT_EQUAL(b.lastValue, 8.0); + + EXPECT_EQUAL(c.observedCount, 2u); + EXPECT_EQUAL(c.sumValue, 11.0); + EXPECT_EQUAL(c.minValue, 1.0); + EXPECT_EQUAL(c.maxValue, 10.0); + EXPECT_EQUAL(c.lastValue, 1.0); + + a.minValue = 8; + a.merge(b); - EXPECT_EQUAL(a.observedCount, 3u); + EXPECT_EQUAL(a.observedCount, 4u); EXPECT_EQUAL(a.sumValue, 24.0); EXPECT_EQUAL(a.minValue, 7.0); EXPECT_EQUAL(a.maxValue, 9.0); EXPECT_EQUAL(a.lastValue, 8.0); + a.merge(b); - EXPECT_EQUAL(a.observedCount, 6u); + EXPECT_EQUAL(a.observedCount, 7u); EXPECT_EQUAL(a.sumValue, 48.0); EXPECT_EQUAL(a.minValue, 7.0); EXPECT_EQUAL(a.maxValue, 9.0); EXPECT_EQUAL(a.lastValue, 8.0); - c.observedCount = 2; - c.sumValue = 11.0; - c.minValue = 1.0; - c.maxValue = 10.0; - c.lastValue = 1.0; - a.merge(c); - EXPECT_EQUAL(a.observedCount, 8u); + EXPECT_EQUAL(a.observedCount, 9u); EXPECT_EQUAL(a.sumValue, 59.0); EXPECT_EQUAL(a.minValue, 1.0); EXPECT_EQUAL(a.maxValue, 10.0); diff --git a/staging_vespalib/src/vespa/vespalib/metrics/CMakeLists.txt b/staging_vespalib/src/vespa/vespalib/metrics/CMakeLists.txt index c56605c9181..d641a768d57 100644 --- a/staging_vespalib/src/vespa/vespalib/metrics/CMakeLists.txt +++ b/staging_vespalib/src/vespa/vespalib/metrics/CMakeLists.txt @@ -8,6 +8,7 @@ vespa_add_library(staging_vespalib_vespalib_metrics OBJECT current_samples.cpp dimension.cpp dummy_metrics_manager.cpp + dummy_time_supplier.cpp gauge_aggregator.cpp gauge.cpp handle.cpp @@ -27,7 +28,10 @@ vespa_add_library(staging_vespalib_vespalib_metrics OBJECT simple_metrics_manager.cpp snapshots.cpp stable_store.cpp + test_time_supplier.cpp ticker_thread.cpp + time_supplier.cpp + wallclock_time_supplier.cpp DEPENDS ) diff --git a/staging_vespalib/src/vespa/vespalib/metrics/bucket.cpp b/staging_vespalib/src/vespa/vespalib/metrics/bucket.cpp index e0d456a5259..9062066fc84 100644 --- a/staging_vespalib/src/vespa/vespalib/metrics/bucket.cpp +++ b/staging_vespalib/src/vespa/vespalib/metrics/bucket.cpp @@ -20,8 +20,10 @@ mergeFromSamples(const StableStore<typename T::sample_type> &source) Map map; source.for_each([&map] (const Sample &sample) { MetricIdentifier id = sample.idx; - auto iter_check = map.emplace(id, Aggregator(id)); - iter_check.first->second.merge(sample); + auto iter_check = map.emplace(id, sample); + if (!iter_check.second) { + iter_check.first->second.merge(sample); + } }); std::vector<typename T::aggregator_type> result; for (const MapValue &entry : map) { @@ -75,9 +77,10 @@ void Bucket::merge(const CurrentSamples &samples) void Bucket::merge(const Bucket &other) { - assert(startTime <= other.startTime); - assert(endTime <= other.endTime); - endTime = other.endTime; + assert(genCnt < other.genCnt); + genCnt = other.genCnt; + startTime = std::min(startTime, other.startTime); + endTime = std::max(endTime, other.endTime); std::vector<CounterAggregator> nextCounters = mergeVectors(counters, other.counters); counters = std::move(nextCounters); diff --git a/staging_vespalib/src/vespa/vespalib/metrics/bucket.h b/staging_vespalib/src/vespa/vespalib/metrics/bucket.h index c35be945343..a0a35e9240d 100644 --- a/staging_vespalib/src/vespa/vespalib/metrics/bucket.h +++ b/staging_vespalib/src/vespa/vespalib/metrics/bucket.h @@ -16,6 +16,7 @@ namespace metrics { // internal struct Bucket { + size_t genCnt; InternalTimeStamp startTime; InternalTimeStamp endTime; std::vector<CounterAggregator> counters; @@ -24,8 +25,9 @@ struct Bucket { void merge(const CurrentSamples &other); void merge(const Bucket &other); - Bucket(InternalTimeStamp started, InternalTimeStamp ended) - : startTime(started), + Bucket(size_t generation, InternalTimeStamp started, InternalTimeStamp ended) + : genCnt(generation), + startTime(started), endTime(ended), counters(), gauges() diff --git a/staging_vespalib/src/vespa/vespalib/metrics/clock.cpp b/staging_vespalib/src/vespa/vespalib/metrics/clock.cpp index 32f7eaec1ad..9d38d48b281 100644 --- a/staging_vespalib/src/vespa/vespalib/metrics/clock.cpp +++ b/staging_vespalib/src/vespa/vespalib/metrics/clock.cpp @@ -7,22 +7,7 @@ namespace metrics { std::chrono::microseconds since_epoch(InternalTimeStamp stamp) { using namespace std::chrono; - using MyInt = microseconds::rep; - - auto before = system_clock::now(); - auto now = steady_clock::now(); - auto after = system_clock::now(); - - MyInt beforems = (time_point_cast<microseconds>(before)).time_since_epoch().count(); - MyInt nowms = (time_point_cast<microseconds>(now)).time_since_epoch().count(); - MyInt afterms = (time_point_cast<microseconds>(after)).time_since_epoch().count(); - - MyInt difference = beforems - nowms; - MyInt adjust = (afterms - beforems) / 2; - - MyInt stampms = stamp.time_since_epoch().count(); - - return microseconds(stampms + difference + adjust); + return duration_cast<microseconds>(stamp.time_since_epoch()); } } // namespace metrics diff --git a/staging_vespalib/src/vespa/vespalib/metrics/clock.h b/staging_vespalib/src/vespa/vespalib/metrics/clock.h index eebfab58336..d29c79bb927 100644 --- a/staging_vespalib/src/vespa/vespalib/metrics/clock.h +++ b/staging_vespalib/src/vespa/vespalib/metrics/clock.h @@ -1,25 +1,12 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. #pragma once -#include <chrono> +#include "wallclock_time_supplier.h" namespace vespalib { namespace metrics { -using InternalClock = std::chrono::steady_clock; -using InternalTimeStamp = std::chrono::time_point<std::chrono::steady_clock, - std::chrono::microseconds>; -using WallClock = std::chrono::system_clock; -using WallTimeStamp = std::chrono::time_point<std::chrono::system_clock, - std::chrono::microseconds>; - -inline InternalTimeStamp now_stamp() -{ - using namespace std::chrono; - return time_point_cast<microseconds>(steady_clock::now()); -} - -std::chrono::microseconds since_epoch(InternalTimeStamp stamp); +using InternalTimeStamp = WallclockTimeSupplier::TimeStamp; } // namespace metrics } // namespace vespalib diff --git a/staging_vespalib/src/vespa/vespalib/metrics/counter.h b/staging_vespalib/src/vespa/vespalib/metrics/counter.h index efa4e29f18a..1af61638d6b 100644 --- a/staging_vespalib/src/vespa/vespalib/metrics/counter.h +++ b/staging_vespalib/src/vespa/vespalib/metrics/counter.h @@ -25,9 +25,12 @@ public: : _manager(std::move(m)), _id(id) {} + // convenience methods: void add() const { add(1, Point::empty); } void add(Point p) { add(1, p); } - void add(size_t count, Point p = Point::empty) const; + void add(size_t count) const { add(count, Point::empty); } + + void add(size_t count, Point p) const; struct Increment { MetricIdentifier idx; diff --git a/staging_vespalib/src/vespa/vespalib/metrics/counter_aggregator.cpp b/staging_vespalib/src/vespa/vespalib/metrics/counter_aggregator.cpp index 6c7e42da193..5102ebb121b 100644 --- a/staging_vespalib/src/vespa/vespalib/metrics/counter_aggregator.cpp +++ b/staging_vespalib/src/vespa/vespalib/metrics/counter_aggregator.cpp @@ -6,18 +6,11 @@ namespace vespalib { namespace metrics { -CounterAggregator::CounterAggregator(MetricIdentifier id) - : idx(id), count(0) +CounterAggregator::CounterAggregator(const Counter::Increment &increment) + : idx(increment.idx), count(increment.value) {} void -CounterAggregator::merge(const Counter::Increment &increment) -{ - assert(idx == increment.idx); - count += increment.value; -} - -void CounterAggregator::merge(const CounterAggregator &other) { assert(idx == other.idx); diff --git a/staging_vespalib/src/vespa/vespalib/metrics/counter_aggregator.h b/staging_vespalib/src/vespa/vespalib/metrics/counter_aggregator.h index 969c80337a0..995ba07deb3 100644 --- a/staging_vespalib/src/vespa/vespalib/metrics/counter_aggregator.h +++ b/staging_vespalib/src/vespa/vespalib/metrics/counter_aggregator.h @@ -12,9 +12,7 @@ struct CounterAggregator { MetricIdentifier idx; size_t count; - CounterAggregator(MetricIdentifier id); - - void merge(const Counter::Increment &other); + CounterAggregator(const Counter::Increment &other); void merge(const CounterAggregator &other); }; diff --git a/staging_vespalib/src/vespa/vespalib/metrics/current_samples.cpp b/staging_vespalib/src/vespa/vespalib/metrics/current_samples.cpp index 4dbd5b6063e..55211717973 100644 --- a/staging_vespalib/src/vespa/vespalib/metrics/current_samples.cpp +++ b/staging_vespalib/src/vespa/vespalib/metrics/current_samples.cpp @@ -4,11 +4,28 @@ namespace vespalib { namespace metrics { -void swap(CurrentSamples& a, CurrentSamples& b) +using Guard = std::lock_guard<std::mutex>; + +void +CurrentSamples::add(Counter::Increment inc) +{ + Guard guard(lock); + counterIncrements.add(inc); +} + +void +CurrentSamples::sample(Gauge::Measurement value) +{ + Guard guard(lock); + gaugeMeasurements.add(value); +} + +void +CurrentSamples::extract(CurrentSamples &into) { - using std::swap; - swap(a.counterIncrements, b.counterIncrements); - swap(a.gaugeMeasurements, b.gaugeMeasurements); + Guard guard(lock); + swap(into.counterIncrements, counterIncrements); + swap(into.gaugeMeasurements, gaugeMeasurements); } } // namespace vespalib::metrics diff --git a/staging_vespalib/src/vespa/vespalib/metrics/current_samples.h b/staging_vespalib/src/vespa/vespalib/metrics/current_samples.h index 78667a3020a..1b87b135aa2 100644 --- a/staging_vespalib/src/vespa/vespalib/metrics/current_samples.h +++ b/staging_vespalib/src/vespa/vespalib/metrics/current_samples.h @@ -17,17 +17,10 @@ struct CurrentSamples { ~CurrentSamples() {} - void add(Counter::Increment inc) { - std::lock_guard<std::mutex> guard(lock); - counterIncrements.add(inc); - } - void sample(Gauge::Measurement value) { - std::lock_guard<std::mutex> guard(lock); - gaugeMeasurements.add(value); - } + void add(Counter::Increment inc); + void sample(Gauge::Measurement value); + void extract(CurrentSamples &into); }; -void swap(CurrentSamples& a, CurrentSamples& b); - } // namespace vespalib::metrics } // namespace vespalib diff --git a/staging_vespalib/src/vespa/vespalib/metrics/dummy_time_supplier.cpp b/staging_vespalib/src/vespa/vespalib/metrics/dummy_time_supplier.cpp new file mode 100644 index 00000000000..53032d2b0c3 --- /dev/null +++ b/staging_vespalib/src/vespa/vespalib/metrics/dummy_time_supplier.cpp @@ -0,0 +1,2 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +#include "dummy_time_supplier.h" diff --git a/staging_vespalib/src/vespa/vespalib/metrics/dummy_time_supplier.h b/staging_vespalib/src/vespa/vespalib/metrics/dummy_time_supplier.h new file mode 100644 index 00000000000..9e74f1006ce --- /dev/null +++ b/staging_vespalib/src/vespa/vespalib/metrics/dummy_time_supplier.h @@ -0,0 +1,14 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +#pragma once + +namespace vespalib { +namespace metrics { + +struct DummyTimeSupplier { + typedef int TimeStamp; + TimeStamp now_stamp() const { return 0; } + double stamp_to_s(TimeStamp) const { return 0.0; } +}; + +} // namespace vespalib::metrics +} // namespace vespalib diff --git a/staging_vespalib/src/vespa/vespalib/metrics/gauge_aggregator.cpp b/staging_vespalib/src/vespa/vespalib/metrics/gauge_aggregator.cpp index 3e36b4e1b30..d7bafb54d0f 100644 --- a/staging_vespalib/src/vespa/vespalib/metrics/gauge_aggregator.cpp +++ b/staging_vespalib/src/vespa/vespalib/metrics/gauge_aggregator.cpp @@ -6,43 +6,21 @@ namespace vespalib { namespace metrics { -GaugeAggregator::GaugeAggregator(MetricIdentifier id) - : idx(id), - observedCount(0), - sumValue(0.0), - minValue(0.0), - maxValue(0.0), - lastValue(0.0) +GaugeAggregator::GaugeAggregator(const Gauge::Measurement &sample) + : idx(sample.idx), + observedCount(1), + sumValue(sample.value), + minValue(sample.value), + maxValue(sample.value), + lastValue(sample.value) {} void -GaugeAggregator::merge(const Gauge::Measurement &other) -{ - assert(idx == other.idx); - if (observedCount == 0) { - sumValue = other.value; - minValue = other.value; - maxValue = other.value; - } else { - sumValue += other.value; - minValue = std::min(minValue, other.value); - maxValue = std::max(maxValue, other.value); - } - lastValue = other.value; - ++observedCount; -} - -void GaugeAggregator::merge(const GaugeAggregator &other) { assert(idx == other.idx); - if (observedCount == 0) { - minValue = other.minValue; - maxValue = other.maxValue; - } else { - minValue = std::min(minValue, other.minValue); - maxValue = std::max(maxValue, other.maxValue); - } + minValue = std::min(minValue, other.minValue); + maxValue = std::max(maxValue, other.maxValue); sumValue += other.sumValue; lastValue = other.lastValue; observedCount += other.observedCount; diff --git a/staging_vespalib/src/vespa/vespalib/metrics/gauge_aggregator.h b/staging_vespalib/src/vespa/vespalib/metrics/gauge_aggregator.h index 231f5650dca..84772d7fff4 100644 --- a/staging_vespalib/src/vespa/vespalib/metrics/gauge_aggregator.h +++ b/staging_vespalib/src/vespa/vespalib/metrics/gauge_aggregator.h @@ -16,9 +16,7 @@ struct GaugeAggregator { double maxValue; double lastValue; - GaugeAggregator(MetricIdentifier id); - - void merge(const Gauge::Measurement &other); + GaugeAggregator(const Gauge::Measurement &other); void merge(const GaugeAggregator &other); }; diff --git a/staging_vespalib/src/vespa/vespalib/metrics/handle.h b/staging_vespalib/src/vespa/vespalib/metrics/handle.h index cd5d0c9e940..416b50f0bcd 100644 --- a/staging_vespalib/src/vespa/vespalib/metrics/handle.h +++ b/staging_vespalib/src/vespa/vespalib/metrics/handle.h @@ -23,28 +23,28 @@ public: template <typename T> bool -operator< (const Handle<T> &a, const Handle<T> &b) +operator< (const Handle<T> &a, const Handle<T> &b) noexcept { return a.id() < b.id(); } template <typename T> bool -operator> (const Handle<T> &a, const Handle<T> &b) +operator> (const Handle<T> &a, const Handle<T> &b) noexcept { return a.id() > b.id(); } template <typename T> bool -operator== (const Handle<T> &a, const Handle<T> &b) +operator== (const Handle<T> &a, const Handle<T> &b) noexcept { return a.id() == b.id(); } template <typename T> bool -operator!= (const Handle<T> &a, const Handle<T> &b) +operator!= (const Handle<T> &a, const Handle<T> &b) noexcept { return a.id() != b.id(); } diff --git a/staging_vespalib/src/vespa/vespalib/metrics/metric_identifier.h b/staging_vespalib/src/vespa/vespalib/metrics/metric_identifier.h index 0fbff69658e..7565a5ed7ea 100644 --- a/staging_vespalib/src/vespa/vespalib/metrics/metric_identifier.h +++ b/staging_vespalib/src/vespa/vespalib/metrics/metric_identifier.h @@ -14,9 +14,6 @@ struct MetricIdentifier { MetricIdentifier() = delete; - explicit MetricIdentifier(MetricName name) - : _name(name), _point(0) {} - MetricIdentifier(MetricName name, Point point) : _name(name), _point(point) {} @@ -38,16 +35,3 @@ struct MetricIdentifier { } // namespace vespalib::metrics } // namespace vespalib - -namespace std -{ - template<> struct hash<vespalib::metrics::MetricIdentifier> - { - typedef vespalib::metrics::MetricIdentifier argument_type; - typedef std::size_t result_type; - result_type operator()(argument_type const& ident) const noexcept - { - return (ident.point().id() << 20) + ident.name().id(); - } - }; -} diff --git a/staging_vespalib/src/vespa/vespalib/metrics/metric_types.cpp b/staging_vespalib/src/vespa/vespalib/metrics/metric_types.cpp index 1d633553796..1d8b1fe2a01 100644 --- a/staging_vespalib/src/vespa/vespalib/metrics/metric_types.cpp +++ b/staging_vespalib/src/vespa/vespalib/metrics/metric_types.cpp @@ -9,7 +9,7 @@ namespace vespalib { namespace metrics { const char* MetricTypes::_typeNames[] = { - "NONE", + "INVALID", "Counter", "Gauge", "Histogram", @@ -25,10 +25,15 @@ MetricTypes::check(size_t id, const vespalib::string &name, MetricType ty) if (old == ty) { return; } + if (old == MetricType::INVALID) { + _seen[id] = ty; + } LOG(warning, "metric '%s' with different types %s and %s, this will be confusing", name.c_str(), _typeNames[ty], _typeNames[old]); } - assert (_seen.size() == id); + while (_seen.size() < id) { + _seen.push_back(MetricType::INVALID); + } _seen.push_back(ty); } diff --git a/staging_vespalib/src/vespa/vespalib/metrics/point_map.cpp b/staging_vespalib/src/vespa/vespalib/metrics/point_map.cpp index ca62a3a7d6e..6818050233f 100644 --- a/staging_vespalib/src/vespa/vespalib/metrics/point_map.cpp +++ b/staging_vespalib/src/vespa/vespalib/metrics/point_map.cpp @@ -26,9 +26,10 @@ PointMap::operator< (const PointMap &other) const return _map.size() < other._map.size(); } // sizes equal, iterate in parallel - auto m = _map.begin(); - auto o = other._map.begin(); - while (m != _map.end()) { + for (auto m = _map.begin(), o = other._map.begin(); + m != _map.end(); + ++m, ++o) + { const Dimension& d1 = m->first; const Dimension& d2 = o->first; if (d1 != d2) { @@ -37,10 +38,8 @@ PointMap::operator< (const PointMap &other) const const Label &l1 = m->second; const Label &l2 = o->second; if (l1 != l2) { - return l1 != l2; + return l1 < l2; } - ++m; - ++o; } // equal return false; diff --git a/staging_vespalib/src/vespa/vespalib/metrics/simple_metrics_manager.cpp b/staging_vespalib/src/vespa/vespalib/metrics/simple_metrics_manager.cpp index 4d906df8624..ec2b8bbc66f 100644 --- a/staging_vespalib/src/vespa/vespalib/metrics/simple_metrics_manager.cpp +++ b/staging_vespalib/src/vespa/vespalib/metrics/simple_metrics_manager.cpp @@ -9,18 +9,21 @@ namespace metrics { using Guard = std::lock_guard<std::mutex>; +WallclockTimeSupplier timeSupplier; + SimpleMetricsManager::SimpleMetricsManager(const SimpleManagerConfig &config) : _metricNames(), _dimensionNames(), _labelValues(), _pointMaps(), _currentBucket(), - _startTime(now_stamp()), + _startTime(timeSupplier.now_stamp()), _curTime(_startTime), + _collectCnt(0), _buckets(), _firstBucket(0), _maxBuckets(config.sliding_window_seconds), - _totalsBucket(_startTime, _startTime), + _totalsBucket(0, _startTime, _startTime), _ticker(this) { if (_maxBuckets < 1) _maxBuckets = 1; @@ -65,7 +68,7 @@ SimpleMetricsManager::mergeBuckets() Guard bucketsGuard(_bucketsLock); if (_buckets.size() > 0) { InternalTimeStamp startTime = _buckets[_firstBucket].startTime; - Bucket merger(startTime, startTime); + Bucket merger(0, startTime, startTime); for (size_t i = 0; i < _buckets.size(); i++) { size_t off = (_firstBucket + i) % _buckets.size(); merger.merge(_buckets[off]); @@ -73,7 +76,14 @@ SimpleMetricsManager::mergeBuckets() return merger; } // no data - return Bucket(_startTime, _curTime); + return Bucket(0, _startTime, _curTime); +} + +Bucket +SimpleMetricsManager::totalsBucket() +{ + Guard bucketsGuard(_bucketsLock); + return _totalsBucket; } Snapshot @@ -81,10 +91,9 @@ SimpleMetricsManager::snapshotFrom(const Bucket &bucket) { std::vector<PointSnapshot> points; - std::chrono::microseconds s = since_epoch(bucket.startTime); - std::chrono::microseconds e = since_epoch(bucket.endTime); - const double micro = 0.000001; - Snapshot snap(s.count() * micro, e.count() * micro); + double s = timeSupplier.stamp_to_s(bucket.startTime); + double e = timeSupplier.stamp_to_s(bucket.endTime); + Snapshot snap(s, e); { for (size_t i = 0; i < _pointMaps.size(); ++i) { const PointMap::BackingMap &map = _pointMaps.lookup(i).backingMap(); @@ -122,33 +131,27 @@ SimpleMetricsManager::snapshot() Snapshot SimpleMetricsManager::totalSnapshot() { - Guard guard(_bucketsLock); - return snapshotFrom(_totalsBucket); + Bucket totals = totalsBucket(); + return snapshotFrom(totals); } void -SimpleMetricsManager::collectCurrentBucket() +SimpleMetricsManager::collectCurrentBucket(InternalTimeStamp prev, + InternalTimeStamp curr) { - InternalTimeStamp prev = _curTime; - InternalTimeStamp curr = now_stamp(); - CurrentSamples samples; - { - Guard guard(_currentBucket.lock); - swap(samples, _currentBucket); - } - Bucket newBucket(prev, curr); + _currentBucket.extract(samples); + Bucket newBucket(++_collectCnt, prev, curr); newBucket.merge(samples); Guard guard(_bucketsLock); _totalsBucket.merge(newBucket); if (_buckets.size() < _maxBuckets) { - _buckets.emplace_back(std::move(newBucket)); + _buckets.push_back(std::move(newBucket)); } else { _buckets[_firstBucket] = std::move(newBucket); _firstBucket = (_firstBucket + 1) % _buckets.size(); } - _curTime = curr; } Dimension @@ -181,5 +184,15 @@ SimpleMetricsManager::pointFrom(PointMap::BackingMap map) return Point(id); } +void +SimpleMetricsManager::tick() +{ + Guard guard(_tickLock); + InternalTimeStamp prev = _curTime; + InternalTimeStamp curr = timeSupplier.now_stamp(); + collectCurrentBucket(prev, curr); + _curTime = curr; +} + } // namespace vespalib::metrics } // namespace vespalib diff --git a/staging_vespalib/src/vespa/vespalib/metrics/simple_metrics_manager.h b/staging_vespalib/src/vespa/vespalib/metrics/simple_metrics_manager.h index 5f6cb881480..0d855e63660 100644 --- a/staging_vespalib/src/vespa/vespalib/metrics/simple_metrics_manager.h +++ b/staging_vespalib/src/vespa/vespalib/metrics/simple_metrics_manager.h @@ -49,14 +49,17 @@ private: InternalTimeStamp _curTime; std::mutex _bucketsLock; + size_t _collectCnt; std::vector<Bucket> _buckets; size_t _firstBucket; size_t _maxBuckets; Bucket _totalsBucket; + std::mutex _tickLock; TickerThread _ticker; - void collectCurrentBucket(); // called once per second from another thread + void collectCurrentBucket(InternalTimeStamp prev, InternalTimeStamp curr); Bucket mergeBuckets(); + Bucket totalsBucket(); Snapshot snapshotFrom(const Bucket &bucket); SimpleMetricsManager(const SimpleManagerConfig &config); @@ -82,7 +85,7 @@ public: _currentBucket.sample(value); } - void tick() { collectCurrentBucket(); } + void tick(); // called once per second from another thread }; } // namespace vespalib::metrics diff --git a/staging_vespalib/src/vespa/vespalib/metrics/test_time_supplier.cpp b/staging_vespalib/src/vespa/vespalib/metrics/test_time_supplier.cpp new file mode 100644 index 00000000000..9916b7f8884 --- /dev/null +++ b/staging_vespalib/src/vespa/vespalib/metrics/test_time_supplier.cpp @@ -0,0 +1,2 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +#include "test_time_supplier.h" diff --git a/staging_vespalib/src/vespa/vespalib/metrics/test_time_supplier.h b/staging_vespalib/src/vespa/vespalib/metrics/test_time_supplier.h new file mode 100644 index 00000000000..33142b3202d --- /dev/null +++ b/staging_vespalib/src/vespa/vespalib/metrics/test_time_supplier.h @@ -0,0 +1,20 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +#pragma once + +#include <cstddef> + +namespace vespalib { +namespace metrics { + +class TestTimeSupplier { +private: + mutable size_t _cnt; +public: + typedef size_t TimeStamp; + TimeStamp now_stamp() const { return ++_cnt; } + double stamp_to_s(TimeStamp stamp) const { return (double)stamp; } + TestTimeSupplier() : _cnt(0) {} +}; + +} // namespace vespalib::metrics +} // namespace vespalib diff --git a/staging_vespalib/src/vespa/vespalib/metrics/ticker_thread.cpp b/staging_vespalib/src/vespa/vespalib/metrics/ticker_thread.cpp index dfed3bd053d..3e05b9e45e4 100644 --- a/staging_vespalib/src/vespa/vespalib/metrics/ticker_thread.cpp +++ b/staging_vespalib/src/vespa/vespalib/metrics/ticker_thread.cpp @@ -10,12 +10,6 @@ namespace vespalib { namespace metrics { void -TickerThread::doTickerLoop(TickerThread *me) -{ - me->tickerLoop(); -} - -void TickerThread::tickerLoop() { const std::chrono::seconds oneSec{1}; @@ -31,11 +25,13 @@ TickerThread::tickerLoop() void TickerThread::stop() { - std::unique_lock<std::mutex> locker(_lock); - _runFlag.store(false); - _cond.notify_all(); - locker.unlock(); - _thread.join(); + if (_runFlag) { + std::unique_lock<std::mutex> locker(_lock); + _runFlag.store(false); + _cond.notify_all(); + locker.unlock(); + _thread.join(); + } } diff --git a/staging_vespalib/src/vespa/vespalib/metrics/ticker_thread.h b/staging_vespalib/src/vespa/vespalib/metrics/ticker_thread.h index e00b68e8486..5ecb0714d2c 100644 --- a/staging_vespalib/src/vespa/vespalib/metrics/ticker_thread.h +++ b/staging_vespalib/src/vespa/vespalib/metrics/ticker_thread.h @@ -19,15 +19,14 @@ private: std::condition_variable _cond; std::thread _thread; - static void doTickerLoop(TickerThread *me); void tickerLoop(); public: TickerThread(SimpleMetricsManager * owner) : _owner(owner), _runFlag(true), - _thread(doTickerLoop, this) + _thread(&TickerThread::tickerLoop, this) {} - ~TickerThread() {} + ~TickerThread() { stop(); } void stop(); }; diff --git a/staging_vespalib/src/vespa/vespalib/metrics/time_supplier.cpp b/staging_vespalib/src/vespa/vespalib/metrics/time_supplier.cpp new file mode 100644 index 00000000000..9d708e7f9be --- /dev/null +++ b/staging_vespalib/src/vespa/vespalib/metrics/time_supplier.cpp @@ -0,0 +1,20 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +#include "time_supplier.h" +#include <stdlib.h> + +namespace vespalib::metrics { + +// these should never be used for anything: + +TimeSupplier::TimeStamp +TimeSupplier::now_stamp() const +{ abort(); } + +double +TimeSupplier::stamp_to_s(TimeStamp) const +{ abort(); } + +TimeSupplier::TimeSupplier() +{ abort(); } + +} // namespace vespalib::metrics diff --git a/staging_vespalib/src/vespa/vespalib/metrics/time_supplier.h b/staging_vespalib/src/vespa/vespalib/metrics/time_supplier.h new file mode 100644 index 00000000000..578a97f1a90 --- /dev/null +++ b/staging_vespalib/src/vespa/vespalib/metrics/time_supplier.h @@ -0,0 +1,32 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +#pragma once + +namespace vespalib::metrics { + +/** + * This is the API you need to implement in order to be used as a TimeSupplier. + **/ +struct TimeSupplier { + /** + * provide a type that can be used for time stamping, like a time_point: + **/ + typedef int TimeStamp; + + /** + * provide a method that can be called to get a time stamp for "now": + **/ + TimeStamp now_stamp() const; + + /** + * provide a method that can convert the time stamp (obtained from + * above method) into seconds since 1970, as a double: + **/ + double stamp_to_s(TimeStamp) const; + + /** + * constructor will usually be trivial. + **/ + TimeSupplier(); +}; + +} // namespace vespalib::metrics diff --git a/staging_vespalib/src/vespa/vespalib/metrics/wallclock_time_supplier.cpp b/staging_vespalib/src/vespa/vespalib/metrics/wallclock_time_supplier.cpp new file mode 100644 index 00000000000..bb2393d0867 --- /dev/null +++ b/staging_vespalib/src/vespa/vespalib/metrics/wallclock_time_supplier.cpp @@ -0,0 +1,2 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +#include "wallclock_time_supplier.h" diff --git a/staging_vespalib/src/vespa/vespalib/metrics/wallclock_time_supplier.h b/staging_vespalib/src/vespa/vespalib/metrics/wallclock_time_supplier.h new file mode 100644 index 00000000000..8fa30fc799c --- /dev/null +++ b/staging_vespalib/src/vespa/vespalib/metrics/wallclock_time_supplier.h @@ -0,0 +1,23 @@ +// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +#pragma once + +#include <chrono> + +namespace vespalib { +namespace metrics { + +class WallclockTimeSupplier { +private: + using Clock = std::chrono::system_clock; + using seconds = std::chrono::duration<double>; +public: + typedef Clock::time_point TimeStamp; + TimeStamp now_stamp() const { return Clock::now(); } + double stamp_to_s(TimeStamp stamp) const { + seconds s = stamp.time_since_epoch(); + return s.count(); + } +}; + +} // namespace vespalib::metrics +} // namespace vespalib |