Merge pull request #28244 from vespa-engine/vekterli/do-not-present-v1-metrics-until-valid-snapshot

Do not return State V1 metrics until at least one metric snapshot has been created

6 files changed, 37 insertions, 3 deletions

diff --git a/metrics/src/tests/metricmanagertest.cpp b/metrics/src/tests/metricmanagertest.cpp
index 9e6b0f40be3..a6dd141576f 100644
--- a/metrics/src/tests/metricmanagertest.cpp
+++ b/metrics/src/tests/metricmanagertest.cpp
@@ -462,6 +462,7 @@ TEST_F(MetricManagerTest, test_snapshots)
     {
         MetricLockGuard lockGuard(mm.getMetricLock());
         mm.registerMetric(lockGuard, mySet.set);
+        EXPECT_FALSE(mm.any_snapshots_taken(lockGuard)); // well-defined prior to init()
     }
     mm.init(ConfigUri("raw:"
                       "consumer[2]\n"
@@ -474,6 +475,7 @@ TEST_F(MetricManagerTest, test_snapshots)
     MetricNameVisitor visitor;
     {
         MetricLockGuard lockGuard(mm.getMetricLock());
+        EXPECT_FALSE(mm.any_snapshots_taken(lockGuard)); // No snapshots yet
         mm.visit(lockGuard, mm.getActiveMetrics(lockGuard), visitor, "snapper");
         const MetricManager::ConsumerSpec * consumerSpec = mm.getConsumerSpec(lockGuard, "snapper");
         EXPECT_EQ(std::string("\n"
@@ -506,6 +508,10 @@ TEST_F(MetricManagerTest, test_snapshots)
     ASSERT_VALUES(mm,  5 * 60s, "2,4,4,1,7,9,1,1,8,2,10");
     ASSERT_VALUES(mm, 60 * 60s, "");
     ASSERT_VALUES(mm,  0 * 60s, "2,4,4,1,7,9,1,1,8,2,10");
+    {
+        auto guard = mm.getMetricLock();
+        EXPECT_TRUE(mm.any_snapshots_taken(guard)); // At least one snapshot has been taken
+    }
 
     // Adding metrics done in second five minute period. Total should
     // be updated to account for both
@@ -567,6 +573,14 @@ TEST_F(MetricManagerTest, test_json_output)
                       "consumer[0].tags[1]\n"
                       "consumer[0].tags[0] snaptest\n"));
 
+    {
+        // No snapshots have been taken yet, so the non-total getMetrics call should return
+        // the empty string (i.e. no metrics produced).
+        metrics::StateApiAdapter adapter(mm);
+        auto json_str = adapter.getMetrics("snapper");
+        EXPECT_EQ(json_str, "");
+    }
+
     takeSnapshots(mm, 1000);
 
     // Adding metrics to have some values in them
diff --git a/metrics/src/vespa/metrics/metricmanager.cpp b/metrics/src/vespa/metrics/metricmanager.cpp
index e399a23b0b9..fa18ddc383b 100644
--- a/metrics/src/vespa/metrics/metricmanager.cpp
+++ b/metrics/src/vespa/metrics/metricmanager.cpp
@@ -168,6 +168,11 @@ MetricManager::isInitialized() const {
     return static_cast<bool>(_configHandle);
 }
 
+bool
+MetricManager::any_snapshots_taken(const MetricLockGuard&) const noexcept {
+    return (!_snapshots.empty() && _snapshots[0]->current_is_assigned());
+}
+
 void
 MetricManager::init(const config::ConfigUri & uri, bool startThread)
 {
@@ -795,6 +800,7 @@ MetricManager::takeSnapshots(const MetricLockGuard & guard, system_time timeToPr
     _activeMetrics.addToSnapshot(firstTarget, false, timeToProcess);
     _activeMetrics.addToSnapshot(*_totalMetrics, false, timeToProcess);
     _activeMetrics.reset(timeToProcess);
+    _snapshots[0]->tag_current_as_assigned();
     LOG(debug, "After snapshotting, active metrics goes from %s to %s, and 5 minute metrics goes from %s to %s.",
         to_string(_activeMetrics.getFromTime()).c_str(), to_string(_activeMetrics.getToTime()).c_str(),
         to_string(firstTarget.getFromTime()).c_str(), to_string(firstTarget.getToTime()).c_str());
diff --git a/metrics/src/vespa/metrics/metricmanager.h b/metrics/src/vespa/metrics/metricmanager.h
index 81d0c581370..cfb3ab2137a 100644
--- a/metrics/src/vespa/metrics/metricmanager.h
+++ b/metrics/src/vespa/metrics/metricmanager.h
@@ -267,6 +267,8 @@ public:
 
     bool isInitialized() const;
 
+    [[nodiscard]] bool any_snapshots_taken(const MetricLockGuard&) const noexcept;
+
 private:
     void takeSnapshots(const MetricLockGuard &, system_time timeToProcess);
 
diff --git a/metrics/src/vespa/metrics/metricsnapshot.cpp b/metrics/src/vespa/metrics/metricsnapshot.cpp
index 08ab1be3033..104ad858e43 100644
--- a/metrics/src/vespa/metrics/metricsnapshot.cpp
+++ b/metrics/src/vespa/metrics/metricsnapshot.cpp
@@ -78,12 +78,15 @@ MetricSnapshotSet::MetricSnapshotSet(const Metric::String& name, system_time::du
     : _count(count),
       _builderCount(0),
       _current(std::make_unique<MetricSnapshot>(name, period, source, snapshotUnsetMetrics)),
-      _building(count == 1 ? nullptr : new MetricSnapshot(name, period, source, snapshotUnsetMetrics))
+      _building(count == 1 ? nullptr : new MetricSnapshot(name, period, source, snapshotUnsetMetrics)),
+      _current_is_assigned(false)
 {
     _current->reset();
     if (_building) _building->reset();
 }
 
+MetricSnapshotSet::~MetricSnapshotSet() = default;
+
 MetricSnapshot&
 MetricSnapshotSet::getNextTarget()
 {
diff --git a/metrics/src/vespa/metrics/metricsnapshot.h b/metrics/src/vespa/metrics/metricsnapshot.h
index e2cedf018c9..a6a68b43015 100644
--- a/metrics/src/vespa/metrics/metricsnapshot.h
+++ b/metrics/src/vespa/metrics/metricsnapshot.h
@@ -77,9 +77,11 @@ class MetricSnapshotSet {
                             // building instance.
     std::unique_ptr<MetricSnapshot> _current; // The last full period
     std::unique_ptr<MetricSnapshot> _building; // The building period
+    bool _current_is_assigned;
 public:
     MetricSnapshotSet(const Metric::String& name, system_time::duration period, uint32_t count,
                       const MetricSet& source, bool snapshotUnsetMetrics);
+    ~MetricSnapshotSet();
 
     const Metric::String& getName() const { return _current->getName(); }
     system_time::duration getPeriod() const { return _current->getPeriod(); }
@@ -109,6 +111,13 @@ public:
     void recreateSnapshot(const MetricSet& metrics, bool copyUnset);
     void addMemoryUsage(MemoryConsumption&) const;
     void setFromTime(system_time fromTime);
+
+    [[nodiscard]] bool current_is_assigned() const noexcept {
+        return _current_is_assigned;
+    }
+    void tag_current_as_assigned() noexcept {
+        _current_is_assigned = true;
+    }
 };
 
 } // metrics
diff --git a/metrics/src/vespa/metrics/state_api_adapter.cpp b/metrics/src/vespa/metrics/state_api_adapter.cpp
index 136ccf6e06a..6c0cb9a6013 100644
--- a/metrics/src/vespa/metrics/state_api_adapter.cpp
+++ b/metrics/src/vespa/metrics/state_api_adapter.cpp
@@ -11,8 +11,8 @@ StateApiAdapter::getMetrics(const vespalib::string &consumer)
 {
     MetricLockGuard guard(_manager.getMetricLock());
     auto periods = _manager.getSnapshotPeriods(guard);
-    if (periods.empty()) {
-        return ""; // no configuration yet
+    if (periods.empty() || !_manager.any_snapshots_taken(guard)) {
+        return ""; // no configuration or snapshots yet
     }
     const MetricSnapshot &snapshot(_manager.getMetricSnapshot(guard, periods[0]));
     vespalib::asciistream json;