wgtmac commented on code in PR #589:
URL: https://github.com/apache/iceberg-cpp/pull/589#discussion_r3309710818


##########
src/iceberg/metrics/timer.h:
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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
+ *
+ *   http://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 <atomic>
+#include <chrono>
+#include <cstdint>
+#include <string_view>
+#include <type_traits>
+
+#include "iceberg/iceberg_export.h"
+
+namespace iceberg {
+
+/// \brief Abstract timer for measuring operation durations.
+///
+/// Use Start() to obtain a Timed RAII
+/// guard that records the elapsed duration when it goes out of scope.
+class ICEBERG_EXPORT Timer {
+ public:
+  /// \brief RAII guard that records elapsed time into the owning Timer on 
destruction.
+  class ICEBERG_EXPORT Timed {
+   public:
+    explicit Timed(Timer& timer);
+    ~Timed();
+
+    Timed(const Timed&) = delete;
+    Timed& operator=(const Timed&) = delete;
+    Timed(Timed&& other) noexcept;
+    Timed& operator=(Timed&& other) noexcept;
+
+    /// \brief Explicitly stop timing and record the duration.
+    ///
+    /// Subsequent calls (including the destructor) are no-ops.
+    void Stop();
+
+   private:
+    Timer* timer_;
+    std::chrono::steady_clock::time_point start_;
+    bool stopped_ = false;
+  };
+
+  virtual ~Timer() = default;
+
+  /// \brief Number of timing recordings made so far.
+  virtual int64_t Count() const = 0;
+
+  /// \brief Total accumulated duration across all recordings.
+  virtual std::chrono::nanoseconds TotalDuration() const = 0;
+
+  /// \brief Record a nanosecond duration directly.
+  ///
+  /// Use the template overload below to record
+  /// any std::chrono duration type with automatic unit conversion.
+  virtual void Record(std::chrono::nanoseconds duration) = 0;
+
+  /// \brief Record a duration of any chrono type, converting to nanoseconds.
+  template <typename Rep, typename Period>
+  void Record(std::chrono::duration<Rep, Period> duration) {
+    Record(std::chrono::duration_cast<std::chrono::nanoseconds>(duration));
+  }
+
+  /// \brief Return the time unit used by this timer (always "nanoseconds").
+  virtual std::string_view Unit() const { return "nanoseconds"; }
+
+  /// \brief Return true if this timer is a no-op.
+  virtual bool IsNoop() const { return false; }
+
+  /// \brief Start timing and return a RAII Timed guard.
+  ///
+  /// The elapsed duration is recorded into this timer when the Timed guard is
+  /// destroyed or Stop() is called.
+  Timed Start();
+
+  /// \brief Execute a callable, record its wall-clock duration, and return 
its result.
+  template <typename Callable>
+  auto Time(Callable&& fn) {
+    auto timed = Start();
+    if constexpr (std::is_void_v<std::invoke_result_t<Callable>>) {
+      std::forward<Callable>(fn)();
+      timed.Stop();
+    } else {
+      auto result = std::forward<Callable>(fn)();
+      timed.Stop();
+      return result;
+    }
+  }
+
+  /// \brief Return a shared no-op timer singleton.
+  static Timer& Noop();

Review Comment:
   Is it more flexible to return a `std::shared_ptr<Timer>`?



##########
src/iceberg/metrics/counter.h:
##########
@@ -0,0 +1,100 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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
+ *
+ *   http://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 <atomic>
+#include <cstdint>
+#include <string_view>
+#include <utility>
+
+#include "iceberg/iceberg_export.h"
+
+namespace iceberg {
+
+/// \brief Unit for a Counter metric.
+enum class CounterUnit {
+  kCount,
+  kBytes,
+  kUndefined,
+};
+
+/// \brief String representation of a CounterUnit.
+ICEBERG_EXPORT constexpr std::string_view ToString(CounterUnit unit) noexcept {
+  switch (unit) {
+    case CounterUnit::kCount:
+      return "count";
+    case CounterUnit::kBytes:
+      return "bytes";
+    case CounterUnit::kUndefined:
+      return "undefined";
+  }
+  std::unreachable();
+}
+
+/// \brief Parse a CounterUnit from a string.
+///
+/// \param s The string to parse ("count", "bytes", or "undefined").
+/// \return The CounterUnit, or CounterUnit::kCount if unrecognized.
+ICEBERG_EXPORT constexpr CounterUnit CounterUnitFromString(std::string_view s) 
noexcept {
+  if (s == "bytes") return CounterUnit::kBytes;
+  if (s == "undefined") return CounterUnit::kUndefined;
+  return CounterUnit::kCount;
+}
+
+/// \brief Abstract counter for tracking event totals.
+class ICEBERG_EXPORT Counter {
+ public:
+  virtual ~Counter() = default;
+
+  /// \brief Increment the counter by 1.
+  virtual void Increment() = 0;
+
+  /// \brief Increment the counter by the given amount.
+  virtual void Increment(int64_t amount) = 0;
+
+  /// \brief Return the current count.
+  virtual int64_t Value() const = 0;
+
+  /// \brief Return the unit for this counter.
+  virtual CounterUnit Unit() const { return CounterUnit::kCount; }
+
+  /// \brief Return true if this counter is a no-op.
+  virtual bool IsNoop() const { return false; }
+
+  /// \brief Return a shared no-op counter singleton.
+  static Counter& Noop();

Review Comment:
   ditto, should we use `std::shared_ptr<Counter>` here?



##########
src/iceberg/metrics/timer.h:
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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
+ *
+ *   http://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 <atomic>
+#include <chrono>
+#include <cstdint>
+#include <string_view>
+#include <type_traits>
+
+#include "iceberg/iceberg_export.h"
+
+namespace iceberg {
+
+/// \brief Abstract timer for measuring operation durations.
+///
+/// Use Start() to obtain a Timed RAII
+/// guard that records the elapsed duration when it goes out of scope.
+class ICEBERG_EXPORT Timer {
+ public:
+  /// \brief RAII guard that records elapsed time into the owning Timer on 
destruction.
+  class ICEBERG_EXPORT Timed {
+   public:
+    explicit Timed(Timer& timer);
+    ~Timed();
+
+    Timed(const Timed&) = delete;
+    Timed& operator=(const Timed&) = delete;
+    Timed(Timed&& other) noexcept;
+    Timed& operator=(Timed&& other) noexcept;
+
+    /// \brief Explicitly stop timing and record the duration.
+    ///
+    /// Subsequent calls (including the destructor) are no-ops.
+    void Stop();
+
+   private:
+    Timer* timer_;
+    std::chrono::steady_clock::time_point start_;
+    bool stopped_ = false;
+  };
+
+  virtual ~Timer() = default;
+
+  /// \brief Number of timing recordings made so far.
+  virtual int64_t Count() const = 0;
+
+  /// \brief Total accumulated duration across all recordings.
+  virtual std::chrono::nanoseconds TotalDuration() const = 0;
+
+  /// \brief Record a nanosecond duration directly.
+  ///
+  /// Use the template overload below to record
+  /// any std::chrono duration type with automatic unit conversion.
+  virtual void Record(std::chrono::nanoseconds duration) = 0;
+
+  /// \brief Record a duration of any chrono type, converting to nanoseconds.
+  template <typename Rep, typename Period>
+  void Record(std::chrono::duration<Rep, Period> duration) {
+    Record(std::chrono::duration_cast<std::chrono::nanoseconds>(duration));
+  }
+
+  /// \brief Return the time unit used by this timer (always "nanoseconds").
+  virtual std::string_view Unit() const { return "nanoseconds"; }
+
+  /// \brief Return true if this timer is a no-op.
+  virtual bool IsNoop() const { return false; }
+
+  /// \brief Start timing and return a RAII Timed guard.
+  ///
+  /// The elapsed duration is recorded into this timer when the Timed guard is
+  /// destroyed or Stop() is called.
+  Timed Start();
+
+  /// \brief Execute a callable, record its wall-clock duration, and return 
its result.
+  template <typename Callable>
+  auto Time(Callable&& fn) {
+    auto timed = Start();
+    if constexpr (std::is_void_v<std::invoke_result_t<Callable>>) {
+      std::forward<Callable>(fn)();
+      timed.Stop();
+    } else {
+      auto result = std::forward<Callable>(fn)();
+      timed.Stop();
+      return result;
+    }
+  }

Review Comment:
   
   ```suggestion
     decltype(auto) Time(Callable&& fn) {
       auto timed = Start();
       return std::forward<Callable>(fn)();
     }
   ```
   
   How about this? It can support a wider range of return types.



##########
src/iceberg/metrics/counter.h:
##########
@@ -0,0 +1,100 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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
+ *
+ *   http://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 <atomic>
+#include <cstdint>
+#include <string_view>
+#include <utility>
+
+#include "iceberg/iceberg_export.h"
+
+namespace iceberg {
+
+/// \brief Unit for a Counter metric.
+enum class CounterUnit {
+  kCount,
+  kBytes,
+  kUndefined,
+};
+
+/// \brief String representation of a CounterUnit.
+ICEBERG_EXPORT constexpr std::string_view ToString(CounterUnit unit) noexcept {
+  switch (unit) {
+    case CounterUnit::kCount:
+      return "count";
+    case CounterUnit::kBytes:
+      return "bytes";
+    case CounterUnit::kUndefined:
+      return "undefined";
+  }
+  std::unreachable();
+}
+
+/// \brief Parse a CounterUnit from a string.
+///
+/// \param s The string to parse ("count", "bytes", or "undefined").
+/// \return The CounterUnit, or CounterUnit::kCount if unrecognized.
+ICEBERG_EXPORT constexpr CounterUnit CounterUnitFromString(std::string_view s) 
noexcept {
+  if (s == "bytes") return CounterUnit::kBytes;
+  if (s == "undefined") return CounterUnit::kUndefined;
+  return CounterUnit::kCount;
+}
+
+/// \brief Abstract counter for tracking event totals.
+class ICEBERG_EXPORT Counter {
+ public:
+  virtual ~Counter() = default;
+
+  /// \brief Increment the counter by 1.
+  virtual void Increment() = 0;

Review Comment:
   Does this have to be a pure virtual function? Can't we directly call 
`Increment(1)`?



##########
src/iceberg/metrics/counter.h:
##########
@@ -0,0 +1,100 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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
+ *
+ *   http://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 <atomic>
+#include <cstdint>
+#include <string_view>
+#include <utility>
+
+#include "iceberg/iceberg_export.h"
+
+namespace iceberg {
+
+/// \brief Unit for a Counter metric.
+enum class CounterUnit {
+  kCount,
+  kBytes,
+  kUndefined,
+};
+
+/// \brief String representation of a CounterUnit.
+ICEBERG_EXPORT constexpr std::string_view ToString(CounterUnit unit) noexcept {
+  switch (unit) {
+    case CounterUnit::kCount:
+      return "count";
+    case CounterUnit::kBytes:
+      return "bytes";
+    case CounterUnit::kUndefined:
+      return "undefined";
+  }
+  std::unreachable();
+}
+
+/// \brief Parse a CounterUnit from a string.
+///
+/// \param s The string to parse ("count", "bytes", or "undefined").
+/// \return The CounterUnit, or CounterUnit::kCount if unrecognized.
+ICEBERG_EXPORT constexpr CounterUnit CounterUnitFromString(std::string_view s) 
noexcept {
+  if (s == "bytes") return CounterUnit::kBytes;
+  if (s == "undefined") return CounterUnit::kUndefined;
+  return CounterUnit::kCount;
+}
+
+/// \brief Abstract counter for tracking event totals.
+class ICEBERG_EXPORT Counter {
+ public:
+  virtual ~Counter() = default;
+
+  /// \brief Increment the counter by 1.
+  virtual void Increment() = 0;
+
+  /// \brief Increment the counter by the given amount.
+  virtual void Increment(int64_t amount) = 0;
+
+  /// \brief Return the current count.
+  virtual int64_t Value() const = 0;

Review Comment:
   No strong opinion but by convention trivial getters usually use snake case 
like `value()` and `unit()`.



##########
src/iceberg/metrics/scan_report.h:
##########
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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
+ *
+ *   http://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 <memory>
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+#include "iceberg/constants.h"
+#include "iceberg/expression/expression.h"
+#include "iceberg/iceberg_export.h"
+#include "iceberg/metrics/metrics_context.h"
+#include "iceberg/metrics/metrics_types.h"
+#include "iceberg/metrics/timer.h"
+
+namespace iceberg {
+
+// Forward declaration: ScanMetrics is defined later in this header.
+class ScanMetrics;
+
+/// \brief Immutable snapshot of scan metrics for use in ScanReport.
+///
+/// Populated by ScanMetrics::ToResult() after a scan completes.
+struct ICEBERG_EXPORT ScanMetricsResult {
+  /// \brief Total planning duration (count of recordings + accumulated 
nanoseconds).
+  TimerResult total_planning_duration;
+  /// \brief Number of data files included in the scan result.
+  CounterResult result_data_files;
+  /// \brief Number of delete files included in the scan result.
+  CounterResult result_delete_files;
+  /// \brief Number of data manifests whose files were read (not skipped).
+  CounterResult scanned_data_manifests;
+  /// \brief Number of delete manifests whose files were read (not skipped).
+  CounterResult scanned_delete_manifests;
+  /// \brief Total number of data manifests in the snapshot.
+  CounterResult total_data_manifests;
+  /// \brief Total number of delete manifests in the snapshot.
+  CounterResult total_delete_manifests;
+  /// \brief Total byte size of all result data files.
+  CounterResult total_file_size_in_bytes;
+  /// \brief Total byte size of all result delete files.
+  CounterResult total_delete_file_size_in_bytes;
+  /// \brief Number of data manifests skipped by partition/stats pruning.
+  CounterResult skipped_data_manifests;
+  /// \brief Number of delete manifests skipped by partition/stats pruning.
+  CounterResult skipped_delete_manifests;
+  /// \brief Number of individual data files skipped by stats pruning.
+  CounterResult skipped_data_files;
+  /// \brief Number of individual delete files skipped by stats pruning.
+  CounterResult skipped_delete_files;
+  /// \brief Number of indexed delete files (positional or DV) in the result.
+  CounterResult indexed_delete_files;
+  /// \brief Number of equality delete files in the result.
+  CounterResult equality_delete_files;
+  /// \brief Number of positional delete files in the result.
+  CounterResult positional_delete_files;
+  /// \brief Number of deletion vectors in the result.
+  CounterResult dvs;
+
+  bool operator==(const ScanMetricsResult&) const = default;
+
+  /// \brief Build a ScanMetricsResult from live scan metrics.
+  static ScanMetricsResult From(const ScanMetrics& scan_metrics);
+};
+
+/// \brief Live scan metrics collected during a table scan operation.
+///
+/// Holds named Counter and Timer instances obtained from a MetricsContext.
+/// Call Of() at the start of a scan to obtain an instrumented instance, then
+/// increment counters and start/stop the planning timer as the scan proceeds.
+/// Call ToResult() at the end to obtain the serialisable ScanMetricsResult.
+class ICEBERG_EXPORT ScanMetrics {
+ public:
+  /// \brief Create a ScanMetrics instance backed by the given MetricsContext.
+  static ScanMetrics Of(MetricsContext& context);

Review Comment:
   Should we return `std::unique_ptr<ScanMetrics>` instead? Downstream is free 
to choose unique_ptr or shared_ptr then.



##########
src/iceberg/metrics/scan_report.cc:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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
+ *
+ *   http://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.
+ */
+
+#include "iceberg/metrics/scan_report.h"
+
+namespace iceberg {
+
+ScanMetrics ScanMetrics::Of(MetricsContext& context) {
+  ScanMetrics m;
+  m.total_planning_duration = context.GetTimer("totalPlanningDuration");

Review Comment:
   Should it be `total-planning-duration`?



##########
src/iceberg/metrics/scan_report.h:
##########
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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
+ *
+ *   http://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 <memory>
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+#include "iceberg/constants.h"
+#include "iceberg/expression/expression.h"
+#include "iceberg/iceberg_export.h"
+#include "iceberg/metrics/metrics_context.h"
+#include "iceberg/metrics/metrics_types.h"
+#include "iceberg/metrics/timer.h"
+
+namespace iceberg {
+
+// Forward declaration: ScanMetrics is defined later in this header.
+class ScanMetrics;
+
+/// \brief Immutable snapshot of scan metrics for use in ScanReport.
+///
+/// Populated by ScanMetrics::ToResult() after a scan completes.
+struct ICEBERG_EXPORT ScanMetricsResult {
+  /// \brief Total planning duration (count of recordings + accumulated 
nanoseconds).
+  TimerResult total_planning_duration;
+  /// \brief Number of data files included in the scan result.
+  CounterResult result_data_files;
+  /// \brief Number of delete files included in the scan result.
+  CounterResult result_delete_files;
+  /// \brief Number of data manifests whose files were read (not skipped).
+  CounterResult scanned_data_manifests;
+  /// \brief Number of delete manifests whose files were read (not skipped).
+  CounterResult scanned_delete_manifests;
+  /// \brief Total number of data manifests in the snapshot.
+  CounterResult total_data_manifests;
+  /// \brief Total number of delete manifests in the snapshot.
+  CounterResult total_delete_manifests;
+  /// \brief Total byte size of all result data files.
+  CounterResult total_file_size_in_bytes;
+  /// \brief Total byte size of all result delete files.
+  CounterResult total_delete_file_size_in_bytes;
+  /// \brief Number of data manifests skipped by partition/stats pruning.
+  CounterResult skipped_data_manifests;
+  /// \brief Number of delete manifests skipped by partition/stats pruning.
+  CounterResult skipped_delete_manifests;
+  /// \brief Number of individual data files skipped by stats pruning.
+  CounterResult skipped_data_files;
+  /// \brief Number of individual delete files skipped by stats pruning.
+  CounterResult skipped_delete_files;
+  /// \brief Number of indexed delete files (positional or DV) in the result.
+  CounterResult indexed_delete_files;
+  /// \brief Number of equality delete files in the result.
+  CounterResult equality_delete_files;
+  /// \brief Number of positional delete files in the result.
+  CounterResult positional_delete_files;
+  /// \brief Number of deletion vectors in the result.
+  CounterResult dvs;
+
+  bool operator==(const ScanMetricsResult&) const = default;
+
+  /// \brief Build a ScanMetricsResult from live scan metrics.
+  static ScanMetricsResult From(const ScanMetrics& scan_metrics);
+};
+
+/// \brief Live scan metrics collected during a table scan operation.
+///
+/// Holds named Counter and Timer instances obtained from a MetricsContext.
+/// Call Of() at the start of a scan to obtain an instrumented instance, then
+/// increment counters and start/stop the planning timer as the scan proceeds.
+/// Call ToResult() at the end to obtain the serialisable ScanMetricsResult.
+class ICEBERG_EXPORT ScanMetrics {
+ public:
+  /// \brief Create a ScanMetrics instance backed by the given MetricsContext.
+  static ScanMetrics Of(MetricsContext& context);

Review Comment:
   Do we want to make the default ctor private?



##########
src/iceberg/metrics/json_serde.h:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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
+ *
+ *   http://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
+
+/// \file iceberg/metrics/json_serde.h
+/// \brief JSON serialization and deserialization for metrics report types.
+
+#include <nlohmann/json_fwd.hpp>
+
+#include "iceberg/iceberg_export.h"
+#include "iceberg/metrics/commit_report.h"
+#include "iceberg/metrics/scan_report.h"
+#include "iceberg/result.h"
+
+namespace iceberg {

Review Comment:
   Rename this file to `json_serde_internal.h` to not install it by accident.



##########
src/iceberg/metrics/scan_report.cc:
##########
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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
+ *
+ *   http://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.
+ */
+
+#include "iceberg/metrics/scan_report.h"
+
+namespace iceberg {
+
+ScanMetrics ScanMetrics::Of(MetricsContext& context) {
+  ScanMetrics m;
+  m.total_planning_duration = context.GetTimer("totalPlanningDuration");

Review Comment:
   Same for all below. CommitMetrics are aligned.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to