From 231c77574ab123e256b81d20ccf2e0e2699c9fea Mon Sep 17 00:00:00 2001 From: Adam Wegrzynek Date: Thu, 29 Nov 2018 14:44:16 +0100 Subject: [PATCH] Remove mutex from Metric class (#95) --- CMakeLists.txt | 4 +- README.md | 1 - .../{11-AutoUpdate.cxx => 9-AutoUpdate.cxx} | 3 - examples/9-Timer.cxx | 21 ------- include/Monitoring/ComplexMetric.h | 60 +++++++++++++++++++ include/Monitoring/Metric.h | 15 +---- include/Monitoring/Monitoring.h | 22 +------ src/ComplexMetric.cxx | 49 +++++++++++++++ src/Metric.cxx | 29 --------- src/Monitoring.cxx | 42 +++---------- test/testMonitoring.cxx | 8 --- 11 files changed, 122 insertions(+), 132 deletions(-) rename examples/{11-AutoUpdate.cxx => 9-AutoUpdate.cxx} (86%) delete mode 100644 examples/9-Timer.cxx create mode 100644 include/Monitoring/ComplexMetric.h create mode 100644 src/ComplexMetric.cxx diff --git a/CMakeLists.txt b/CMakeLists.txt index 9e66f19e1..daf66e8c5 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -67,6 +67,7 @@ set(INCLUDE_OUTPUT_DIRECTORY "${PROJECT_BINARY_DIR}/include") set(SRCS src/Monitoring.cxx src/Metric.cxx + src/ComplexMetric.cxx src/Backends/InfluxDB.cxx src/Backends/Flume.cxx src/Backends/StdOut.cxx @@ -145,9 +146,8 @@ set(EXAMPLES examples/6-Increment.cxx examples/7-Latency.cxx examples/8-Multiple.cxx - examples/9-Timer.cxx + examples/9-AutoUpdate.cxx examples/10-Buffering.cxx - examples/11-AutoUpdate.cxx ) foreach (example ${EXAMPLES}) diff --git a/README.md b/README.md index ef5643a36..6052df907 100644 --- a/README.md +++ b/README.md @@ -113,7 +113,6 @@ Each backend has its default verbosity (see backend in [Monitoring instance](#mo ### Customized metrics Two additional methods can be chained the to `send(Metric&& metric)` in order to __insert custom tags__ or __set custom timestamp__: + `addTags(std::vector&& tags)` - + `setTimestamp(std::chrono::time_point& timestamp)` See how it works in the example: [examples/2-TaggedMetrics.cxx](examples/2-TaggedMetrics.cxx), [examples/3-UserDefinedTimestamp.cxx](examples/3-UserDefinedTimestamp.cxx). diff --git a/examples/11-AutoUpdate.cxx b/examples/9-AutoUpdate.cxx similarity index 86% rename from examples/11-AutoUpdate.cxx rename to examples/9-AutoUpdate.cxx index 81606700d..c1b86fabc 100644 --- a/examples/11-AutoUpdate.cxx +++ b/examples/9-AutoUpdate.cxx @@ -10,9 +10,6 @@ using namespace o2::monitoring; int main() { auto monitoring = MonitoringFactory::Get("stdout://"); - // Enable periodical value pushing (default every 1s) - monitoring->enableAutoPush(); - // Get reference to metrics auto& qcMetric = monitoring->getAutoPushMetric("qcMetric"); auto& qcMetric2 = monitoring->getAutoPushMetric("qcMetric2"); diff --git a/examples/9-Timer.cxx b/examples/9-Timer.cxx deleted file mode 100644 index 050f306db..000000000 --- a/examples/9-Timer.cxx +++ /dev/null @@ -1,21 +0,0 @@ -/// -/// \file 9-Timer.cxx -/// \author Adam Wegrzynek -/// - -#include "Monitoring/MonitoringFactory.h" -#include - -using Monitoring = o2::monitoring::MonitoringFactory; - -int main() { - // Configure monitoring - // Pass string with list of URLs as parameter - auto monitoring = Monitoring::Get("stdout://"); - - // Time the execution of the code below - monitoring->startTimer("measureSleep"); - using namespace std::chrono_literals; - std::this_thread::sleep_for(1s); - monitoring->stopAndSendTimer("measureSleep"); -} diff --git a/include/Monitoring/ComplexMetric.h b/include/Monitoring/ComplexMetric.h new file mode 100644 index 000000000..0e98bcbe8 --- /dev/null +++ b/include/Monitoring/ComplexMetric.h @@ -0,0 +1,60 @@ +/// +/// \file ComplexMetric.h +/// \author Adam Wegrzynek +/// + +#ifndef ALICEO2_MONITORING_CORE_COMPLEXMETRIC_H +#define ALICEO2_MONITORING_CORE_COMPLEXMETRIC_H + +#include "Metric.h" +#include + +namespace o2 +{ +/// ALICE O2 Monitoring system +namespace monitoring +{ + +// \brief Extends metric to value setter +class ComplexMetric : public o2::monitoring::Metric +{ + public: + /// Integer metric construtor + /// \param value metric value (int) + /// \param name metric name + ComplexMetric(int value, const std::string& name); + + /// String metric construtor + /// \param value metric value (string) + /// \param name the metric name + ComplexMetric(std::string value, const std::string& name); + + /// Double metric constructor + /// \param value metric value (double) + /// \param name metric name + ComplexMetric(double value, const std::string& name); + + /// uint64_t metric constructor + /// \param value metric value (uint64_t) + /// \param name metric name + ComplexMetric(uint64_t value, const std::string& name); + + /// boost variant metric constructor, required by derived metrics logic + /// \param value metric value (boost variant) + /// \param name metric name + ComplexMetric(boost::variant< int, std::string, double, uint64_t >, const std::string& name); + + /// Default destructor + ~ComplexMetric() = default; + + // Resets metric's timestamp + void resetTimestamp(); + + /// Assign operator overload, assignes new values to the metric object + ComplexMetric& operator=(const boost::variant< int, std::string, double, uint64_t >& value); +}; + +} // namespace monitoring +} // namespace o2 + +#endif // ALICEO2_MONITORING_CORE_COMPLEXMETRIC_H diff --git a/include/Monitoring/Metric.h b/include/Monitoring/Metric.h index 829a7c1b4..7793566ac 100644 --- a/include/Monitoring/Metric.h +++ b/include/Monitoring/Metric.h @@ -8,7 +8,6 @@ #include #include -#include #include #include #include "Tag.h" @@ -58,15 +57,6 @@ class Metric /// Default destructor ~Metric() = default; - /// Copy initialization - Metric(const Metric& other); - - /// Copy assignment - Metric& operator=(Metric const& other); - - /// Assign operator overload, assignes new values to the metric object - Metric& operator=(const boost::variant< int, std::string, double, uint64_t >& value); - /// Name getter /// \return metric name std::string getName() const; @@ -96,7 +86,7 @@ class Metric /// return timestamp as std::chrono::system_clock static auto getCurrentTimestamp() -> decltype(std::chrono::system_clock::now()); - private: + protected: /// Metric value boost::variant< int, std::string, double, uint64_t > mValue; @@ -108,9 +98,6 @@ class Metric /// Metric tags std::vector tagSet; - - /// Mutex for accesing metric value - mutable std::mutex mValueMutex; }; } // namespace monitoring diff --git a/include/Monitoring/Monitoring.h b/include/Monitoring/Monitoring.h index 21236e750..e34c990e5 100644 --- a/include/Monitoring/Monitoring.h +++ b/include/Monitoring/Monitoring.h @@ -17,6 +17,7 @@ #include #include +#include "Monitoring/ComplexMetric.h" #include "Monitoring/Backend.h" #include "Monitoring/DerivedMetrics.h" #include "Monitoring/ProcessMonitor.h" @@ -76,16 +77,6 @@ class Monitoring /// \param interval refresh interval void enableProcessMonitoring(const unsigned int interval = 5); - /// Starts timing - /// Sets a start timestamp and timeout - /// \param name metric name - void startTimer(std::string name); - - /// Stops timing - /// Sets stop timestamp, calculates delta and sends value - /// \param name metric name - void stopAndSendTimer(std::string name); - /// Flushes metric buffer (this can also happen when buffer is full) void flushBuffer(); @@ -101,11 +92,7 @@ class Monitoring /// Returns a metric which will be periodically sent to backends /// \param name metric name /// \return periodically send metric - Metric& getAutoPushMetric(std::string name); - - /// Enables periodical push interval - /// \param interval interval in seconds - void enableAutoPush(const unsigned int interval = 1); + ComplexMetric& getAutoPushMetric(std::string name, unsigned int interval = 1); private: /// Derived metrics handler @@ -115,9 +102,6 @@ class Monitoring /// Vector of backends (where metrics are passed to) std::vector > mBackends; - /// List of timers - std::unordered_map > mTimers; - /// Pushes metric to all backends or to the buffer void pushToBackends(Metric&& metric); @@ -143,7 +127,7 @@ class Monitoring unsigned int mBufferSize; /// Store for automatically pushed metrics - std::deque mPushStore; + std::deque mPushStore; /// Process monitor interval std::atomic mProcessMonitoringInterval; diff --git a/src/ComplexMetric.cxx b/src/ComplexMetric.cxx new file mode 100644 index 000000000..7227f661e --- /dev/null +++ b/src/ComplexMetric.cxx @@ -0,0 +1,49 @@ +/// +/// \file ComplexMetric.cxx +/// \author Adam Wegrzynek +/// + +#include "Monitoring/ComplexMetric.h" + +#include +#include +#include + +namespace o2 +{ +/// ALICE O2 Monitoring system +namespace monitoring +{ + +ComplexMetric::ComplexMetric(int value, const std::string& name) : + Metric(value, name) +{} + +ComplexMetric::ComplexMetric(std::string value, const std::string& name) : + Metric(value, name) +{} + +ComplexMetric::ComplexMetric(double value, const std::string& name) : + Metric(value, name) +{} + +ComplexMetric::ComplexMetric(uint64_t value, const std::string& name) : + Metric(value, name) +{} + +ComplexMetric::ComplexMetric(boost::variant< int, std::string, double, uint64_t > value, const std::string& name) : + Metric(value, name) +{} + +void ComplexMetric::resetTimestamp() +{ + mTimestamp = Metric::getCurrentTimestamp(); +} + +ComplexMetric& ComplexMetric::operator=(const boost::variant< int, std::string, double, uint64_t >& value) { + mValue = value; + return *this; +} + +} // namespace monitoring +} // namespace o2 diff --git a/src/Metric.cxx b/src/Metric.cxx index 99812b997..5d8583d0f 100644 --- a/src/Metric.cxx +++ b/src/Metric.cxx @@ -50,40 +50,11 @@ Metric::Metric(boost::variant< int, std::string, double, uint64_t > value, const mValue(value), mName(name), mTimestamp(timestamp) {} -Metric::Metric(const Metric& other) -{ - std::lock_guard lock(other.mValueMutex); - mName = other.mName; - mValue = other.mValue; - mTimestamp = other.mTimestamp; - tagSet = other.tagSet; -} - -Metric& Metric::operator=(Metric const& other) -{ - if (&other != this) { - std::unique_lock lockThis(mValueMutex, std::defer_lock); - std::unique_lock lockOther(other.mValueMutex, std::defer_lock); - std::lock(lockThis, lockOther); - - mName = other.mName; - mValue = other.mValue; - mTimestamp = other.mTimestamp; - tagSet = other.tagSet; - } - return *this; -} - boost::variant< int, std::string, double, uint64_t > Metric::getValue() const { return mValue; } -Metric& Metric::operator=(const boost::variant< int, std::string, double, uint64_t >& value) { - mValue = value; - return *this; -} - Metric&& Metric::addTags(std::vector&& tags) { tagSet = std::move(tags); diff --git a/src/Monitoring.cxx b/src/Monitoring.cxx index b1301aee3..bbcfa2ba8 100644 --- a/src/Monitoring.cxx +++ b/src/Monitoring.cxx @@ -63,28 +63,6 @@ void Monitoring::enableProcessMonitoring(const unsigned int interval) { #endif } -void Monitoring::startTimer(std::string name) { - auto search = mTimers.find(name); - if (search == mTimers.end()) { - auto now = std::chrono::steady_clock::now(); - mTimers.insert(std::make_pair(name, now)); - } else { - MonLogger::Get() << "Monitoring timer : Timer for " << name << " already started" << MonLogger::End(); - } -} - -void Monitoring::stopAndSendTimer(std::string name) { - auto search = mTimers.find(name); - if (search != mTimers.end()) { - auto now = std::chrono::duration_cast (std::chrono::steady_clock::now().time_since_epoch()).count(); - auto start = std::chrono::duration_cast (search->second.time_since_epoch()).count(); - uint64_t duration = now - start; - send({duration, name}); - } else { - MonLogger::Get() << "Monitoring timer : Cannot stop " << name << " timer as it hasn't started" << MonLogger::End(); - } -} - void Monitoring::addGlobalTag(std::string name, std::string value) { for (auto& backend: mBackends) { @@ -124,7 +102,8 @@ void Monitoring::pushLoop() if (mAutoPushInterval != 0 && (loopCount % (mAutoPushInterval*10)) == 0) { std::vector metrics; - for (auto& metric : mPushStore) { + for (auto metric : mPushStore) { + metric.resetTimestamp(); metrics.push_back(metric); } send(std::move(metrics)); @@ -134,10 +113,12 @@ void Monitoring::pushLoop() } } -Metric& Monitoring::getAutoPushMetric(std::string name) +ComplexMetric& Monitoring::getAutoPushMetric(std::string name, unsigned int interval) { - if (mAutoPushInterval == 0) { - MonLogger::Get() << "[WARN] AutoPush is not enabled" << MonLogger::End(); + if (!mMonitorRunning) { + mMonitorRunning = true; + mMonitorThread = std::thread(&Monitoring::pushLoop, this); + mAutoPushInterval = interval; } mPushStore.emplace_back(boost::variant< int, std::string, double, uint64_t > {}, name); return mPushStore.back(); @@ -166,15 +147,6 @@ void Monitoring::debug(Metric&& metric) } } -void Monitoring::enableAutoPush(unsigned int interval) -{ - if (!mMonitorRunning) { - mMonitorRunning = true; - mMonitorThread = std::thread(&Monitoring::pushLoop, this); - } - mAutoPushInterval = interval; -} - void Monitoring::pushToBackends(Metric&& metric) { if (mBuffering) { diff --git a/test/testMonitoring.cxx b/test/testMonitoring.cxx index 882cf5c55..1e86e5618 100644 --- a/test/testMonitoring.cxx +++ b/test/testMonitoring.cxx @@ -43,17 +43,9 @@ BOOST_AUTO_TEST_CASE(buffering) monitoring->flushBuffer(); } -BOOST_AUTO_TEST_CASE(testTimer) -{ - auto monitoring = Monitoring::Get("stdout://"); - monitoring->startTimer("test"); - monitoring->stopAndSendTimer("timer"); -} - BOOST_AUTO_TEST_CASE(testPush) { auto monitoring = Monitoring::Get("stdout://"); - monitoring->enableAutoPush(); auto& qcMetric = monitoring->getAutoPushMetric("qcMetric"); auto& qcMetric2 = monitoring->getAutoPushMetric("qcMetric2"); std::this_thread::sleep_for (std::chrono::milliseconds(1500));