pw_metric: Share MetricWalker

Moves the MetricWalker to a private header and introduces a virtual
MetricWriter interface to share code common between the pwpb and nanopb
MetricService implementations.

Change-Id: Ibcb173e7ce5cb178a0ec66d07de6c44d1101b3b4
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/101380
Reviewed-by: Keir Mierle <keir@google.com>
Commit-Queue: Auto-Submit <auto-submit@pigweed.google.com.iam.gserviceaccount.com>
Pigweed-Auto-Submit: Armando Montanez <amontanez@google.com>

Author: Medha Kini

pw_metric/BUILD.bazel

@@ -50,26 +50,38 @@ pw_cc_library(
     ],
 )
 
+# Common MetricWalker/MetricWriter used by RPC service.
+pw_cc_library(
+    name = "metric_walker",
+    hdrs = ["pw_metric_private/metric_walker.h"],
+    visibility = ["//visibility:private"],
+    deps = [
+        ":pw_metric",
+        "//pw_assert:assert",
+        "//pw_containers",
+        "//pw_status",
+        "//pw_tokenizer",
+    ],
+)
+
 pw_cc_library(
     name = "metric_service_nanopb",
     srcs = ["metric_service_nanopb.cc"],
-    hdrs = [
-        "public/pw_metric/metric_service_nanopb.h",
-    ],
+    hdrs = ["public/pw_metric/metric_service_nanopb.h"],
     deps = [
         ":metric",
+        ":metric_walker",
     ],
 )
 
 pw_cc_library(
     name = "metric_service_pwpb",
     srcs = ["metric_service_pwpb.cc"],
-    hdrs = [
-        "public/pw_metric/metric_service_pwpb.h",
-    ],
+    hdrs = ["public/pw_metric/metric_service_pwpb.h"],
     deps = [
         ":metric",
         ":metric_proto_cc",
+        ":metric_walker",
         "//pw_assert",
         "//pw_bytes",
         "//pw_containers",

pw_metric/BUILD.gn

@@ -62,6 +62,19 @@ pw_proto_library("metric_service_proto") {
 # TODO(keir): Consider moving the nanopb service into the nanopb/ directory
 # instead of having it directly inside pw_metric/.
 
+# Common MetricWalker/MetricWriter used by RPC service.
+pw_source_set("metric_walker") {
+  visibility = [ ":*" ]
+  public = [ "pw_metric_private/metric_walker.h" ]
+  deps = [
+    ":pw_metric",
+    "$dir_pw_assert:assert",
+    "$dir_pw_containers",
+    "$dir_pw_status",
+    "$dir_pw_tokenizer",
+  ]
+}
+
 if (dir_pw_third_party_nanopb != "") {
   pw_source_set("metric_service_nanopb") {
     public_configs = [ ":default_config" ]
@@ -73,6 +86,7 @@ if (dir_pw_third_party_nanopb != "") {
     public = [ "public/pw_metric/metric_service_nanopb.h" ]
     deps = [
       ":metric_service_proto.nanopb_rpc",
+      ":metric_walker",
       "$dir_pw_containers:vector",
       dir_pw_tokenizer,
     ]
@@ -94,6 +108,7 @@ pw_source_set("metric_service_pwpb") {
   public_deps = [
     ":metric_service_proto.pwpb_rpc",
     ":metric_service_proto.raw_rpc",
+    ":metric_walker",
     ":pw_metric",
     "$dir_pw_bytes",
     "$dir_pw_containers",

pw_metric/metric_service_nanopb.cc

@@ -19,23 +19,25 @@
 #include "pw_assert/check.h"
 #include "pw_containers/vector.h"
 #include "pw_metric/metric.h"
+#include "pw_metric_private/metric_walker.h"
 #include "pw_preprocessor/util.h"
 #include "pw_span/span.h"
 
 namespace pw::metric {
 namespace {
 
-class MetricWriter {
+class NanopbMetricWriter : public virtual internal::MetricWriter {
  public:
-  MetricWriter(MetricService::ServerWriter<pw_metric_proto_MetricResponse>&
-                   response_writer)
+  NanopbMetricWriter(
+      MetricService::ServerWriter<pw_metric_proto_MetricResponse>&
+          response_writer)
       : response_(pw_metric_proto_MetricResponse_init_zero),
         response_writer_(response_writer) {}
 
   // TODO(keir): Figure out a pw_rpc mechanism to fill a streaming packet based
   // on transport MTU, rather than having this as a static knob. For example,
   // some transports may be able to fit 30 metrics; others, only 5.
-  void Write(const Metric& metric, const Vector<Token>& path) {
+  Status Write(const Metric& metric, const Vector<Token>& path) override {
     // Nanopb doesn't offer an easy way to do bounds checking, so use span's
     // type deduction magic to figure out the max size.
     span<pw_metric_proto_Metric> metrics(response_.metrics);
@@ -68,6 +70,8 @@ class MetricWriter {
     if (response_.metrics_count == metrics.size()) {
       Flush();
     }
+
+    return OkStatus();
   }
 
   void Flush() {
@@ -84,58 +88,14 @@ class MetricWriter {
   MetricService::ServerWriter<pw_metric_proto_MetricResponse>& response_writer_;
 };
 
-// Walk a metric tree recursively; passing metrics with their path (names) to a
-// metric writer which can consume them.
-//
-// TODO(keir): Generalize this to support a generic visitor.
-class MetricWalker {
- public:
-  MetricWalker(MetricWriter& writer) : writer_(writer) {}
-
-  void Walk(const IntrusiveList<Metric>& metrics) {
-    for (const auto& m : metrics) {
-      ScopedName scoped_name(m.name(), *this);
-      writer_.Write(m, path_);
-    }
-  }
-
-  void Walk(const IntrusiveList<Group>& groups) {
-    for (const auto& g : groups) {
-      Walk(g);
-    }
-  }
-
-  void Walk(const Group& group) {
-    ScopedName scoped_name(group.name(), *this);
-    Walk(group.children());
-    Walk(group.metrics());
-  }
-
- private:
-  // Exists to safely push/pop parent groups from the explicit stack.
-  struct ScopedName {
-    ScopedName(Token name, MetricWalker& rhs) : walker(rhs) {
-      PW_CHECK_INT_LT(walker.path_.size(),
-                      walker.path_.capacity(),
-                      "Metrics are too deep; bump path_ capacity");
-      walker.path_.push_back(name);
-    }
-    ~ScopedName() { walker.path_.pop_back(); }
-    MetricWalker& walker;
-  };
-
-  Vector<Token, 4 /* max depth */> path_;
-  MetricWriter& writer_;
-};
-
 }  // namespace
 
 void MetricService::Get(
     const pw_metric_proto_MetricRequest& /* request */,
     ServerWriter<pw_metric_proto_MetricResponse>& response) {
   // For now, ignore the request and just stream all the metrics back.
-  MetricWriter writer(response);
-  MetricWalker walker(writer);
+  NanopbMetricWriter writer(response);
+  internal::MetricWalker walker(writer);
 
   // This will stream all the metrics in the span of this Get() method call.
   // This will have the effect of blocking the RPC thread until all the metrics
@@ -144,8 +104,8 @@ void MetricService::Get(
   //
   // In the future, this should be replaced with an optional async solution
   // that puts the application in control of when the response batches are sent.
-  walker.Walk(metrics_);
-  walker.Walk(groups_);
+  walker.Walk(metrics_).IgnoreError();
+  walker.Walk(groups_).IgnoreError();
   writer.Flush();
 }
 

pw_metric/metric_service_pwpb.cc

@@ -19,6 +19,7 @@
 #include "pw_assert/check.h"
 #include "pw_containers/vector.h"
 #include "pw_metric/metric.h"
+#include "pw_metric_private/metric_walker.h"
 #include "pw_metric_proto/metric_service.pwpb.h"
 #include "pw_preprocessor/util.h"
 #include "pw_rpc/raw/server_reader_writer.h"
@@ -33,17 +34,18 @@ constexpr size_t kMaxNumPackedEntries = 3;
 
 namespace {
 
-class MetricWriter {
+class PwpbMetricWriter : public virtual internal::MetricWriter {
  public:
-  MetricWriter(span<std::byte> response, rpc::RawServerWriter& response_writer)
+  PwpbMetricWriter(span<std::byte> response,
+                   rpc::RawServerWriter& response_writer)
       : response_(response),
         response_writer_(response_writer),
         encoder_(response) {}
 
   // TODO(keir): Figure out a pw_rpc mechanism to fill a streaming packet based
   // on transport MTU, rather than having this as a static knob. For example,
   // some transports may be able to fit 30 metrics; others, only 5.
-  Status Write(const Metric& metric, const Vector<Token>& path) {
+  Status Write(const Metric& metric, const Vector<Token>& path) override {
     {  // Scope to control proto_encoder lifetime.
 
       // Grab the next available Metric slot to write to in the response.
@@ -86,54 +88,6 @@ class MetricWriter {
   proto::MetricRequest::MemoryEncoder encoder_;
   size_t metrics_count = 0;
 };
-
-// Walk a metric tree recursively; passing metrics with their path (names) to a
-// metric writer which can consume them.
-//
-// TODO(keir): Generalize this to support a generic visitor.
-class MetricWalker {
- public:
-  MetricWalker(MetricWriter& writer) : writer_(writer) {}
-
-  Status Walk(const IntrusiveList<Metric>& metrics) {
-    for (const auto& m : metrics) {
-      ScopedName scoped_name(m.name(), *this);
-      PW_TRY(writer_.Write(m, path_));
-    }
-    return OkStatus();
-  }
-
-  Status Walk(const IntrusiveList<Group>& groups) {
-    for (const auto& g : groups) {
-      PW_TRY(Walk(g));
-    }
-    return OkStatus();
-  }
-
-  Status Walk(const Group& group) {
-    ScopedName scoped_name(group.name(), *this);
-    PW_TRY(Walk(group.children()));
-    PW_TRY(Walk(group.metrics()));
-    return OkStatus();
-  }
-
- private:
-  // Exists to safely push/pop parent groups from the explicit stack.
-  struct ScopedName {
-    ScopedName(Token name, MetricWalker& rhs) : walker(rhs) {
-      PW_CHECK_INT_LT(walker.path_.size(),
-                      walker.path_.capacity(),
-                      "Metrics are too deep; bump path_ capacity");
-      walker.path_.push_back(name);
-    }
-    ~ScopedName() { walker.path_.pop_back(); }
-    MetricWalker& walker;
-  };
-
-  Vector<Token, 4 /* max depth */> path_;
-  MetricWriter& writer_;
-};
-
 }  // namespace
 
 void MetricService::Get(ConstByteSpan /*request*/,
@@ -147,8 +101,8 @@ void MetricService::Get(ConstByteSpan /*request*/,
 
   std::array<std::byte, kEncodeBufferSize> encode_buffer;
 
-  MetricWriter writer(encode_buffer, raw_response);
-  MetricWalker walker(writer);
+  PwpbMetricWriter writer(encode_buffer, raw_response);
+  internal::MetricWalker walker(writer);
 
   // This will stream all the metrics in the span of this Get() method call.
   // This will have the effect of blocking the RPC thread until all the metrics
@@ -159,7 +113,7 @@ void MetricService::Get(ConstByteSpan /*request*/,
   // that puts the application in control of when the response batches are sent.
 
   // Propagate status through walker.
-  auto status = OkStatus();
+  Status status;
   status.Update(walker.Walk(metrics_));
   status.Update(walker.Walk(groups_));
   status.Update(writer.Flush());

pw_metric/pw_metric_private/metric_walker.h

@@ -0,0 +1,75 @@
+// Copyright 2022 The Pigweed Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License"); you may not
+// use this file except in compliance with the License. You may obtain a copy of
+// the License at
+//
+//     https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+// License for the specific language governing permissions and limitations under
+// the License.
+#pragma once
+
+#include "pw_assert/check.h"
+#include "pw_containers/intrusive_list.h"
+#include "pw_containers/vector.h"
+#include "pw_metric/metric.h"
+#include "pw_status/status.h"
+#include "pw_tokenizer/tokenize.h"
+
+namespace pw::metric::internal {
+
+class MetricWriter {
+ public:
+  virtual ~MetricWriter() = default;
+  virtual Status Write(const Metric& metric, const Vector<Token>& path) = 0;
+};
+
+// Walk a metric tree recursively; passing metrics with their path (names) to a
+// MetricWriter that can consume them.
+class MetricWalker {
+ public:
+  MetricWalker(MetricWriter& writer) : writer_(writer) {}
+
+  Status Walk(const IntrusiveList<Metric>& metrics) {
+    for (const auto& m : metrics) {
+      ScopedName scoped_name(m.name(), *this);
+      PW_TRY(writer_.Write(m, path_));
+    }
+    return OkStatus();
+  }
+
+  Status Walk(const IntrusiveList<Group>& groups) {
+    for (const auto& g : groups) {
+      PW_TRY(Walk(g));
+    }
+    return OkStatus();
+  }
+
+  Status Walk(const Group& group) {
+    ScopedName scoped_name(group.name(), *this);
+    PW_TRY(Walk(group.children()));
+    PW_TRY(Walk(group.metrics()));
+    return OkStatus();
+  }
+
+ private:
+  // Exists to safely push/pop parent groups from the explicit stack.
+  struct ScopedName {
+    ScopedName(Token name, MetricWalker& rhs) : walker(rhs) {
+      // Metrics are too deep; bump path_ capacity.
+      PW_ASSERT(walker.path_.size() < walker.path_.capacity());
+      walker.path_.push_back(name);
+    }
+    ~ScopedName() { walker.path_.pop_back(); }
+    MetricWalker& walker;
+  };
+
+  Vector<Token, /*capacity=*/4> path_;
+  MetricWriter& writer_;
+};
+
+}  // namespace pw::metric::internal