Merge pull request #18563 from vespa-engine/toregge/adjust-proton-disk-mem-usage-metrics-reporting

Adjust reporting of disk and memory usage in proton.

10 files changed, 135 insertions, 9 deletions

diff --git a/searchcore/CMakeLists.txt b/searchcore/CMakeLists.txt
index 55b3b66a75c..dfcc7542e09 100644
--- a/searchcore/CMakeLists.txt
+++ b/searchcore/CMakeLists.txt
@@ -143,6 +143,7 @@ vespa_define_module(
     src/tests/proton/reprocessing/reprocessing_runner
     src/tests/proton/server
     src/tests/proton/server/disk_mem_usage_filter
+    src/tests/proton/server/disk_mem_usage_metrics
     src/tests/proton/server/disk_mem_usage_sampler
     src/tests/proton/server/health_adapter
     src/tests/proton/server/memory_flush_config_updater
diff --git a/searchcore/src/tests/proton/server/disk_mem_usage_metrics/CMakeLists.txt b/searchcore/src/tests/proton/server/disk_mem_usage_metrics/CMakeLists.txt
new file mode 100644
index 00000000000..fbd557991c8
--- /dev/null
+++ b/searchcore/src/tests/proton/server/disk_mem_usage_metrics/CMakeLists.txt
@@ -0,0 +1,9 @@
+# Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+vespa_add_executable(searchcore_disk_mem_usage_metrics_test_app TEST
+    SOURCES
+    disk_mem_usage_metrics_test.cpp
+    DEPENDS
+    searchcore_server
+    GTest::GTest
+)
+vespa_add_test(NAME searchcore_disk_mem_usage_metrics_test_app COMMAND searchcore_disk_mem_usage_metrics_test_app)
diff --git a/searchcore/src/tests/proton/server/disk_mem_usage_metrics/disk_mem_usage_metrics_test.cpp b/searchcore/src/tests/proton/server/disk_mem_usage_metrics/disk_mem_usage_metrics_test.cpp
new file mode 100644
index 00000000000..569633c518b
--- /dev/null
+++ b/searchcore/src/tests/proton/server/disk_mem_usage_metrics/disk_mem_usage_metrics_test.cpp
@@ -0,0 +1,39 @@
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
+#include <vespa/searchcore/proton/server/disk_mem_usage_metrics.h>
+#include <vespa/searchcore/proton/server/disk_mem_usage_state.h>
+#include <vespa/vespalib/gtest/gtest.h>
+
+
+using proton::DiskMemUsageMetrics;
+using proton::DiskMemUsageState;
+using proton::ResourceUsageState;
+
+bool
+expect_metrics(double disk_usage, double disk_utilization, double memory_usage, double memory_utilization, const DiskMemUsageMetrics &dm_metrics)
+{
+    bool result = true;
+    EXPECT_DOUBLE_EQ(disk_usage, dm_metrics.get_disk_usage()) << (result = false, "");
+    EXPECT_DOUBLE_EQ(disk_utilization, dm_metrics.get_disk_utilization()) << (result = false, "");
+    EXPECT_DOUBLE_EQ(memory_usage, dm_metrics.get_memory_usage()) << (result = false, "");
+    EXPECT_DOUBLE_EQ(memory_utilization, dm_metrics.get_memory_utilization()) << (result = false, "");
+    return result;
+}
+
+TEST(DiskMemUsageMetricsTest, default_value_is_zero)
+{
+    DiskMemUsageMetrics dm_metrics;
+    EXPECT_TRUE(expect_metrics(0.0, 0.0, 0.0, 0.0, dm_metrics));
+}
+
+TEST(DiskMemUsageMetricsTest, merging_uses_max)
+{
+    DiskMemUsageMetrics dm_metrics({ResourceUsageState(0.5, 0.4), ResourceUsageState(0.5, 0.3)});
+    EXPECT_TRUE(expect_metrics(0.4, 0.8, 0.3, 0.6, dm_metrics));
+    dm_metrics.merge({ResourceUsageState(0.4, 0.4), ResourceUsageState(0.5, 0.4)});
+    EXPECT_TRUE(expect_metrics(0.4, 1.0, 0.4, 0.8, dm_metrics));
+    dm_metrics.merge({ResourceUsageState(0.5, 0.4), ResourceUsageState(0.5, 0.3)});
+    EXPECT_TRUE(expect_metrics(0.4, 1.0, 0.4, 0.8, dm_metrics));
+}
+
+GTEST_MAIN_RUN_ALL_TESTS()
diff --git a/searchcore/src/vespa/searchcore/proton/server/CMakeLists.txt b/searchcore/src/vespa/searchcore/proton/server/CMakeLists.txt
index d2ebb7a58a2..285e259e1a8 100644
--- a/searchcore/src/vespa/searchcore/proton/server/CMakeLists.txt
+++ b/searchcore/src/vespa/searchcore/proton/server/CMakeLists.txt
@@ -11,6 +11,7 @@ vespa_add_library(searchcore_server STATIC
     ddbstate.cpp
     disk_mem_usage_filter.cpp
     disk_mem_usage_forwarder.cpp
+    disk_mem_usage_metrics.cpp
     disk_mem_usage_sampler.cpp
     docstorevalidator.cpp
     document_db_config_owner.cpp
diff --git a/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_filter.cpp b/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_filter.cpp
index ef77081132b..cbd378da9a0 100644
--- a/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_filter.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_filter.cpp
@@ -154,13 +154,14 @@ DiskMemUsageFilter::getDiskUsedRatio(const Guard &guard) const
 DiskMemUsageFilter::DiskMemUsageFilter(const HwInfo &hwInfo)
     : _lock(),
       _hwInfo(hwInfo),
+      _acceptWrite(true),
       _memoryStats(),
       _diskUsedSizeBytes(),
       _transient_memory_usage(0u),
       _config(),
       _state(),
-      _acceptWrite(true),
       _dmstate(),
+      _disk_mem_usage_metrics(),
       _listeners()
 { }
 
@@ -254,6 +255,15 @@ DiskMemUsageFilter::usageState() const
     return _dmstate;
 }
 
+DiskMemUsageMetrics
+DiskMemUsageFilter::get_metrics() const
+{
+    Guard guard(_lock);
+    DiskMemUsageMetrics result(_disk_mem_usage_metrics);
+    _disk_mem_usage_metrics = DiskMemUsageMetrics(_dmstate);
+    return result;
+}
+
 bool
 DiskMemUsageFilter::acceptWriteOperation() const
 {
@@ -295,6 +305,7 @@ DiskMemUsageFilter::notifyDiskMemUsage(const Guard &guard, DiskMemUsageState sta
         return;
     }
     _dmstate = state;
+    _disk_mem_usage_metrics.merge(state);
     for (const auto &listener : _listeners) {
         listener->notifyDiskMemUsage(_dmstate);
     }
diff --git a/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_filter.h b/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_filter.h
index cdb9fc5f4cb..5bec34e51d9 100644
--- a/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_filter.h
+++ b/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_filter.h
@@ -4,6 +4,7 @@
 
 #include "i_disk_mem_usage_notifier.h"
 #include "disk_mem_usage_state.h"
+#include "disk_mem_usage_metrics.h"
 #include <vespa/searchcore/proton/common/hw_info.h>
 #include <vespa/searchcore/proton/persistenceengine/i_resource_write_filter.h>
 #include <vespa/vespalib/util/process_memory_stats.h>
@@ -40,16 +41,18 @@ public:
     };
 
 private:
-    mutable Mutex _lock; // protect _memoryStats, _usedDiskSizeBytes, _config, _state
+    mutable Mutex                _lock;
     HwInfo                       _hwInfo;
+    std::atomic<bool>            _acceptWrite;
+    // Following member variables are protected by _lock
     vespalib::ProcessMemoryStats _memoryStats;
     uint64_t                     _diskUsedSizeBytes;
     size_t                       _transient_memory_usage;
     size_t                       _transient_disk_usage;
     Config                       _config;
     State                        _state;
-    std::atomic<bool>            _acceptWrite;
     DiskMemUsageState            _dmstate;
+    mutable DiskMemUsageMetrics  _disk_mem_usage_metrics;
     std::vector<IDiskMemUsageListener *> _listeners;
 
     void recalcState(const Guard &guard); // called with _lock held
@@ -73,6 +76,7 @@ public:
     Config getConfig() const;
     const HwInfo &getHwInfo() const { return _hwInfo; }
     DiskMemUsageState usageState() const;
+    DiskMemUsageMetrics get_metrics() const;
     bool acceptWriteOperation() const override;
     State getAcceptState() const override;
     void addDiskMemUsageListener(IDiskMemUsageListener *listener) override;
diff --git a/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_metrics.cpp b/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_metrics.cpp
new file mode 100644
index 00000000000..2a97c56e63f
--- /dev/null
+++ b/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_metrics.cpp
@@ -0,0 +1,31 @@
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
+#include "disk_mem_usage_metrics.h"
+#include "disk_mem_usage_state.h"
+#include <algorithm>
+
+namespace proton {
+
+DiskMemUsageMetrics::DiskMemUsageMetrics() noexcept
+    : DiskMemUsageMetrics(DiskMemUsageState())
+{
+}
+
+DiskMemUsageMetrics::DiskMemUsageMetrics(const DiskMemUsageState &usage_state) noexcept
+    : _disk_usage(usage_state.diskState().usage()),
+      _disk_utilization(usage_state.diskState().utilization()),
+      _memory_usage(usage_state.memoryState().usage()),
+      _memory_utilization(usage_state.memoryState().utilization())
+{
+}
+
+void
+DiskMemUsageMetrics::merge(const DiskMemUsageState &usage_state) noexcept
+{
+    _disk_usage = std::max(_disk_usage, usage_state.diskState().usage());
+    _disk_utilization = std::max(_disk_utilization, usage_state.diskState().utilization());
+    _memory_usage = std::max(_memory_usage, usage_state.memoryState().usage());
+    _memory_utilization = std::max(_memory_utilization, usage_state.memoryState().utilization());
+}
+
+}
diff --git a/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_metrics.h b/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_metrics.h
new file mode 100644
index 00000000000..34d3bb36b40
--- /dev/null
+++ b/searchcore/src/vespa/searchcore/proton/server/disk_mem_usage_metrics.h
@@ -0,0 +1,30 @@
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
+#pragma once
+
+namespace proton {
+
+class DiskMemUsageState;
+
+/**
+ * Class containing disk and memory usage in a form suitable for
+ * metrics reporting.
+ */
+class DiskMemUsageMetrics
+{
+    double _disk_usage;
+    double _disk_utilization;
+    double _memory_usage;
+    double _memory_utilization;
+
+public:
+    DiskMemUsageMetrics() noexcept;
+    DiskMemUsageMetrics(const DiskMemUsageState &usage_state) noexcept;
+    void merge(const DiskMemUsageState &usage_state) noexcept;
+    double get_disk_usage() const noexcept { return _disk_usage; }
+    double get_disk_utilization() const noexcept { return _disk_utilization; }
+    double get_memory_usage() const noexcept { return _memory_usage; }
+    double get_memory_utilization() const noexcept { return _memory_utilization; }
+};
+
+} // namespace proton
diff --git a/searchcore/src/vespa/searchcore/proton/server/proton.cpp b/searchcore/src/vespa/searchcore/proton/server/proton.cpp
index adae323c1d9..ef72a52eb02 100644
--- a/searchcore/src/vespa/searchcore/proton/server/proton.cpp
+++ b/searchcore/src/vespa/searchcore/proton/server/proton.cpp
@@ -756,11 +756,11 @@ Proton::updateMetrics(const metrics::MetricLockGuard &)
         }
 
         const DiskMemUsageFilter &usageFilter = _diskMemUsageSampler->writeFilter();
-        DiskMemUsageState usageState = usageFilter.usageState();
-        metrics.resourceUsage.disk.set(usageState.diskState().usage());
-        metrics.resourceUsage.diskUtilization.set(usageState.diskState().utilization());
-        metrics.resourceUsage.memory.set(usageState.memoryState().usage());
-        metrics.resourceUsage.memoryUtilization.set(usageState.memoryState().utilization());
+        auto dm_metrics = usageFilter.get_metrics();
+        metrics.resourceUsage.disk.set(dm_metrics.get_disk_usage());
+        metrics.resourceUsage.diskUtilization.set(dm_metrics.get_disk_utilization());
+        metrics.resourceUsage.memory.set(dm_metrics.get_memory_usage());
+        metrics.resourceUsage.memoryUtilization.set(dm_metrics.get_memory_utilization());
         metrics.resourceUsage.transient_memory.set(usageFilter.get_relative_transient_memory_usage());
         metrics.resourceUsage.transient_disk.set(usageFilter.get_relative_transient_disk_usage());
         metrics.resourceUsage.memoryMappings.set(usageFilter.getMemoryStats().getMappingsCount());
diff --git a/searchcore/src/vespa/searchcore/proton/server/resource_usage_state.h b/searchcore/src/vespa/searchcore/proton/server/resource_usage_state.h
index 29f9959aea9..0b6f34ba055 100644
--- a/searchcore/src/vespa/searchcore/proton/server/resource_usage_state.h
+++ b/searchcore/src/vespa/searchcore/proton/server/resource_usage_state.h
@@ -18,7 +18,7 @@ private:
 
 public:
     ResourceUsageState()
-        : _limit(0),
+        : _limit(1.0),
           _usage(0)
     {
     }