Add stats report daemon to Velox #9653

tanjialiang · 2024-04-29T06:00:51Z

The stats report daemon is used for periodically exporting velox metrics. Current supported metrics are memory related metrics. There will be followups for additional metrics.

netlify · 2024-04-29T06:01:09Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`b7079e2`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6632b73f625e87000849bb94

facebook-github-bot · 2024-04-29T06:03:09Z

@tanjialiang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

xiaoxmeng

@tanjialiang thanks for the change % minors.

xiaoxmeng · 2024-04-29T18:05:50Z

velox/common/base/PeriodicStatsReportDaemon.h

+  // Some cumulative metrics needs this previous state to calculate the delta
+  // for reporting.
+  std::map<std::string, MetricUnion> prevSimpleStatsHolder_;
+  velox::memory::MemoryArbitrator::Stats prevArbitratorStats_;


nit: s/prevArbitratorStats_/lastArbitratorStats_/

velox/common/base/PeriodicStatsReportDaemon.cpp

xiaoxmeng · 2024-04-29T18:07:24Z

velox/common/base/PeriodicStatsReportDaemon.h

+  velox::memory::MemoryArbitrator::Stats prevArbitratorStats_;
+  folly::ThreadedRepeatingFunctionRunner scheduler_;
+};
+} // namespace facebook::velox


Can you provide global utility to start and stop the singleton background daemon? We can do that in followup as well. Thanks!

xiaoxmeng · 2024-04-29T19:14:15Z

velox/common/base/PeriodicStatsReportDaemon.cpp

+}
+
+void PeriodicStatsReportDaemon::addLowFrequencyReports() {
+  static constexpr uint64_t kLowFrequencyReportIntervalMs{60'000};


Could we just let each component decides its own frequency and register separately? Thanks!

xiaoxmeng · 2024-04-29T19:17:08Z

velox/common/base/PeriodicStatsReportDaemon.cpp

+      memoryCacheStats.hitBytes;
+  prevSimpleStatsHolder_[kLastMemoryCacheInserts].int64Value =
+      memoryCacheStats.numNew;
+  prevSimpleStatsHolder_[kLastMemoryCacheEvictions].int64Value =


How about we just record the previous raw stats instead of using prevSimpleStatsHolder_? Thanks!

xiaoxmeng · 2024-04-29T19:18:14Z

velox/common/base/PeriodicStatsReportDaemon.cpp

+  VELOX_CHECK_GE(updatedArbitratorStats, prevArbitratorStats_);
+  const auto deltaArbitratorStats =
+      updatedArbitratorStats - prevArbitratorStats_;
+  REPORT_IF_NOT_ZERO(


Can you check SharedArbitrator code? I think most of metrics have already been reported. And we only need to report the average arbitrator free spaces here? Thanks!

synced offline. Will followup remove the duplicated reported metrics in arbitrator

Let's do in this PR? We can't duplicate the report from velox side.

xiaoxmeng · 2024-04-29T23:11:10Z

velox/common/base/PeriodicStatsReportDaemon.cpp

+    const velox::cache::AsyncDataCache* const asyncDataCache,
+    const velox::memory::MemoryArbitrator* const arbitrator)
+    : memoryAllocator_(memoryAllocator),
+      asyncDataCache_(asyncDataCache),


s/asyncDataCache_/cache_/

xiaoxmeng · 2024-04-30T04:59:18Z

velox/common/base/PeriodicStatsReportDaemon.h

+class PeriodicStatsReportDaemon {
+ public:
+  PeriodicStatsReportDaemon(
+      const velox::memory::MemoryAllocator* const memoryAllocator,


const velox::memory::MemoryAllocator* memoryAllocator

dittos

xiaoxmeng · 2024-04-30T04:59:43Z

velox/common/base/PeriodicStatsReportDaemon.h

+ public:
+  PeriodicStatsReportDaemon(
+      const velox::memory::MemoryAllocator* const memoryAllocator,
+      const velox::cache::AsyncDataCache* const asyncDataCache,


nit: s/memoryAllocator/allocator/ s/asyncDataCache/cache/

xiaoxmeng · 2024-04-30T05:01:21Z

velox/common/base/PeriodicStatsReportDaemon.h

+ private:
+  // Add a task to run periodically.
+  template <typename TFunc>
+  void addTask(TFunc&& func, size_t periodMicros, const std::string& taskName) {


s/periodMicros/peridoicIntervalUs or intervalUs/

xiaoxmeng · 2024-04-30T05:02:21Z

velox/common/base/PeriodicStatsReportDaemon.h

+ private:
+  // Add a task to run periodically.
+  template <typename TFunc>
+  void addTask(TFunc&& func, size_t periodMicros, const std::string& taskName) {


Put taskName first?

name, func, periodicIntervalUs

xiaoxmeng · 2024-04-30T05:03:13Z

velox/common/base/PeriodicStatsReportDaemon.h

+            LOG(ERROR) << "Error running periodic task " << taskName << ": "
+                       << e.what();
+          }
+          return std::chrono::milliseconds(periodMicros / 1000);


If this is always in milliseconds, then how about just pass ms? like intervalMs?

xiaoxmeng · 2024-04-30T05:03:57Z

velox/common/base/PeriodicStatsReportDaemon.h

+  cache::CacheStats lastCacheStats_;
+  cache::SsdCacheStats lastSsdStats_;
+  velox::memory::MemoryArbitrator::Stats lastArbitratorStats_;
+  folly::ThreadedRepeatingFunctionRunner scheduler_;


Put scheduler_ ahead of saved last stats? Thanks!

xiaoxmeng · 2024-04-30T05:06:04Z

velox/common/base/PeriodicStatsReportDaemon.cpp

+      arbitrator_(arbitrator) {}
+
+void PeriodicStatsReportDaemon::start() {
+  addTask(


addAllocatorStatsReport(); addCacheStatsReport(); addArbitratorStatsReport()?

Thanks!

Shall we keep it this way instead of creating 3 methods for 3 one-liner to keep the class a bit simpler?

It is fine. Leave one empty line in between. Thanks!

xiaoxmeng · 2024-04-30T05:07:27Z

velox/common/base/PeriodicStatsReportDaemon.cpp

+    }
+  }
+
+  lastCacheStats_ = cacheStats;


Let's just access ssdStats from cacheStats?

We can't. ssdStats is a shared_ptr

We can't keep a shared_ptr?

xiaoxmeng · 2024-04-30T05:08:10Z

velox/common/base/PeriodicStatsReportDaemon.cpp

+  VELOX_CHECK_GE(updatedArbitratorStats, prevArbitratorStats_);
+  const auto deltaArbitratorStats =
+      updatedArbitratorStats - prevArbitratorStats_;
+  REPORT_IF_NOT_ZERO(


Let's do in this PR? We can't duplicate the report from velox side.

tanjialiang · 2024-04-30T05:20:44Z

Let's do in this PR? We can't duplicate the report from velox side.

@xiaoxmeng We can keep it this way and, when changing to plug this class in with presto-native, we can remove the duplicated ones from presto-native to avoid double reporting.

It can be quite easy to forget to add back these particular scattered lines. What do you think?

xiaoxmeng · 2024-04-30T05:23:30Z

velox/common/base/PeriodicStatsReportDaemon.cpp

+#include "velox/common/memory/Memory.h"
+#include "velox/common/memory/MmapAllocator.h"
+
+namespace {


Move anonymous namespace inside velox namespace

velox/common/base/tests/StatsReporterTest.cpp

xiaoxmeng · 2024-04-30T21:56:05Z

velox/common/base/PeriodicStatsReporter.h

+      const velox::memory::MemoryAllocator* allocator,
+      const velox::cache::AsyncDataCache* cache,
+      const velox::memory::MemoryArbitrator* arbitrator,
+      Options options = Options());


const Options& options

xiaoxmeng · 2024-04-30T21:56:51Z

velox/common/base/PeriodicStatsReporter.cpp

+      cache_(cache),
+      arbitrator_(arbitrator),
+      options_(options) {
+  lastCacheStats_.ssdStats = std::make_shared<cache::SsdCacheStats>();


Why not explicit check if ssd is null in stats update?

we can do that, too

velox/common/base/PeriodicStatsReporter.cpp

xiaoxmeng · 2024-04-30T23:34:15Z

velox/common/base/Counters.h

+constexpr folly::StringPiece kMetricArbitratorArbitrationTimeUs{
+    "velox.arbitrator_arbitration_time_us"};
+
+constexpr folly::StringPiece kMetricArbitratorNumShrunkBytes{


Let's skip reporting the following three for now?

We should keep the migration the same as presto-native and modify the migrated ones in followups if needed

xiaoxmeng · 2024-04-30T23:34:35Z

velox/common/base/Counters.h

@@ -128,4 +119,28 @@ constexpr folly::StringPiece kMetricSpillWriteTimeMs{

 constexpr folly::StringPiece kMetricFileWriterEarlyFlushedRawBytes{
    "velox.file_writer_early_flushed_raw_bytes"};
+
+constexpr folly::StringPiece kMetricArbitratorNumRequests{


Can you update document for the new ones? Thanks!

counter document is in .cpp per Velox style, which is different from presto

xiaoxmeng · 2024-04-30T23:37:13Z

velox/common/base/PeriodicStatsReporter.cpp

+  REPORT_IF_NOT_ZERO(
+      kMetricArbitratorNumReclaimedBytes,
+      deltaArbitratorStats.numReclaimedBytes);
+  REPORT_IF_NOT_ZERO(


We shall report kMetricArbitratorFreeCapacityBytes and kMetricArbitratorFreeReservedCapacityBytes from the latest stats? Thanks!

facebook-github-bot · 2024-05-01T00:09:09Z

@tanjialiang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-05-01T17:10:08Z

@tanjialiang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

xiaoxmeng · 2024-05-01T17:14:13Z

velox/docs/monitoring/metrics.rst

@@ -113,8 +113,8 @@ Memory Management
       is not sufficient. It excludes counting instances where the operator is in a
       non-reclaimable state due to currently being on-thread and running or being already
       cancelled.
-   * - arbitrator_requests_count


Let's keep the original names?

Summary: The stats report daemon is used for periodically exporting velox metrics. Current supported metrics are memory related metrics. There will be followups for additional metrics. Pull Request resolved: facebookincubator#9653 Reviewed By: xiaoxmeng Differential Revision: D56690811 Pulled By: tanjialiang

facebook-github-bot · 2024-05-01T18:51:58Z

This pull request was exported from Phabricator. Differential Revision: D56690811

Summary: The stats report daemon is used for periodically exporting velox metrics. Current supported metrics are memory related metrics. There will be followups for additional metrics. Pull Request resolved: facebookincubator#9653 Reviewed By: xiaoxmeng Differential Revision: D56690811 Pulled By: tanjialiang

facebook-github-bot · 2024-05-01T21:42:23Z

This pull request was exported from Phabricator. Differential Revision: D56690811

facebook-github-bot · 2024-05-02T00:20:55Z

@tanjialiang merged this pull request in ebcbec7.

conbench-facebook · 2024-05-02T00:59:46Z

Conbench analyzed the 1 benchmark run on commit ebcbec72.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Summary: The stats report daemon is used for periodically exporting velox metrics. Current supported metrics are memory related metrics. There will be followups for additional metrics. Pull Request resolved: facebookincubator#9653 Reviewed By: xiaoxmeng Differential Revision: D56690811 Pulled By: tanjialiang fbshipit-source-id: e6f7236df9ea1445355f72b6f94b52704e0e1f4e

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 29, 2024

tanjialiang force-pushed the stats_daemon branch from e6473b4 to 63e6cf3 Compare April 29, 2024 06:02

tanjialiang force-pushed the stats_daemon branch from 63e6cf3 to 96b73b9 Compare April 29, 2024 06:32

xiaoxmeng reviewed Apr 29, 2024

View reviewed changes

tanjialiang force-pushed the stats_daemon branch from 96b73b9 to 585b8b4 Compare April 29, 2024 23:48

tanjialiang requested a review from xiaoxmeng April 30, 2024 00:00

xiaoxmeng reviewed Apr 30, 2024

View reviewed changes

tanjialiang force-pushed the stats_daemon branch 2 times, most recently from 1cc913b to dd1db8a Compare April 30, 2024 05:22

xiaoxmeng reviewed Apr 30, 2024

View reviewed changes

tanjialiang force-pushed the stats_daemon branch from dd1db8a to 55ca641 Compare April 30, 2024 05:25

xiaoxmeng reviewed Apr 30, 2024

View reviewed changes

velox/common/base/tests/StatsReporterTest.cpp Outdated Show resolved Hide resolved

tanjialiang force-pushed the stats_daemon branch 2 times, most recently from 0aae33e to 05854c7 Compare April 30, 2024 06:41

xiaoxmeng reviewed Apr 30, 2024

View reviewed changes

tanjialiang force-pushed the stats_daemon branch 3 times, most recently from 3782229 to f1d8d68 Compare April 30, 2024 23:13

xiaoxmeng reviewed Apr 30, 2024

View reviewed changes

tanjialiang force-pushed the stats_daemon branch 2 times, most recently from 7ca5de5 to c346bc4 Compare May 1, 2024 00:07

xiaoxmeng approved these changes May 1, 2024

View reviewed changes

tanjialiang force-pushed the stats_daemon branch from c346bc4 to 5dbb2d4 Compare May 1, 2024 18:51

facebook-github-bot added the fb-exported label May 1, 2024

tanjialiang force-pushed the stats_daemon branch from 5dbb2d4 to b7079e2 Compare May 1, 2024 21:42

facebook-github-bot closed this in ebcbec7 May 2, 2024

facebook-github-bot added the Merged label May 2, 2024

Add stats report daemon to Velox #9653

Add stats report daemon to Velox #9653

Conversation

tanjialiang commented Apr 29, 2024

netlify bot commented Apr 29, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

facebook-github-bot commented Apr 29, 2024

xiaoxmeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanjialiang commented Apr 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented May 1, 2024

facebook-github-bot commented May 1, 2024

Choose a reason for hiding this comment

facebook-github-bot commented May 1, 2024

facebook-github-bot commented May 1, 2024

facebook-github-bot commented May 2, 2024

conbench-facebook bot commented May 2, 2024

netlify bot commented Apr 29, 2024 •

edited

Loading