feat: metrics reporting for scan and commit by evindj · Pull Request #589 · apache/iceberg-cpp

evindj · 2026-03-11T20:28:15Z

Initial commit for addressing #533

wgtmac · 2026-03-20T12:45:44Z

Let me know when ready for review :)

evindj · 2026-03-21T06:46:44Z

Let me know when ready for review :)

for sure ! most likely tomorrow

wgtmac · 2026-03-27T08:10:39Z

Thanks for updating this! I still need to take some time to get familiar with the Java implementation and its API in the rest spec before reviewing it.

wgtmac

Thanks for working on this! I have some general questions on the design, especially on the capability of customization. IMO we can focus on the API design for now and later integrate with other classes. Please let me know what you think.

wgtmac · 2026-03-29T06:56:10Z

+struct ICEBERG_EXPORT ScanReport {
+  /// \brief The fully qualified name of the table that was scanned.
+  std::string table_name;
+


Why some metrics have blank lines in between but others don't? I think we can remove all these blank lines to be compact.

wgtmac · 2026-03-29T06:56:38Z

+  int64_t snapshot_id = -1;
+
+  /// \brief Filter expression used in the scan, if any.
+  std::string filter;


Should this be std::shared_ptr<Expression>?

wgtmac · 2026-03-31T09:37:23Z

+namespace iceberg {
+
+/// \brief Duration type for metrics reporting in milliseconds.
+using DurationMs = std::chrono::milliseconds;


This hard-codes our reported duration unit to be milliseconds, which violates the spec I think?

wgtmac · 2026-03-31T09:38:36Z

+  std::string filter;
+
+  /// \brief Schema ID.
+  int32_t schema_id = -1;


We are missing some fields like projectedFieldIds and projectedFieldNames from the Java implementation. I think they are required by the REST spec: https://github.com/apache/iceberg/blob/149cc464f9b7df800cc5718af725983473819504/open-api/rest-catalog-open-api.yaml#L3990-L4023

wgtmac · 2026-03-31T09:40:29Z

+  std::string table_name;
+
+  /// \brief The snapshot ID created by this commit.
+  int64_t snapshot_id = -1;


Please use kInvalidSnapshotId defined in the iceberg/constant.h

wgtmac · 2026-03-31T09:41:18Z

+  /// \brief Total number of data manifests.
+  int64_t total_data_manifests = 0;
+
+  /// \brief Number of data manifests that were skipped.


The comment is inconsistent.

wgtmac · 2026-03-31T09:41:28Z

+  int64_t skipped_data_files = 0;
+
+  /// \brief Number of data manifests that were skipped.
+  int64_t skipped_delete_files = 0;


wgtmac · 2026-03-31T09:43:17Z

+  int32_t schema_id = -1;
+
+  /// \brief Total duration of the entire scan operation.
+  DurationMs total_duration{0};


Should we remove this as this is not defined by the Java implementation?

wgtmac · 2026-03-31T10:00:01Z

+///
+/// This variant type allows handling both report types uniformly through
+/// the MetricsReporter interface.
+using MetricsReport = std::variant<ScanReport, CommitReport>;


If we define MetricsReport as a std::variant, we cannot support customizing metrics report. For example, engines may have more metrics to report than defined by the Java implementation. Even the REST spec does not explicitly define what keys are required.

Instead, should we define the MetricsReport like below?

struct MetricsReport { std::string kind; // can be "scan" or "commit", or whatever customized std::unordered_map<std::string, CounterResult> counter_results; std::unordered_map<std::string, TimerResult> timer_results; };

What do you think?

After thinking more on this, I'm fine to use your current approach to define ScanReport and CommitReport with explicit fields. MetricsReports are collected by this library so users do not have the flexibility to customize them. We only need to customize MetricsReporter.

I like the bag of keys approach because it provides for customization without necessarily having to require compilation. let me think a bit more about the entire thing, I might reach out on slack for a quick sync.

wgtmac · 2026-03-31T10:01:44Z

+  ///
+  /// \param reporter_type Case-insensitive type identifier (e.g., "noop").
+  /// \param factory Factory function that produces the reporter.
+  static void Register(std::string_view reporter_type, MetricsReporterFactory factory);


How do we support the Java parity CompositeMetricsReporter? It would be useful if we want to report metrics to multiple sinks.

evindj · 2026-03-31T12:42:16Z

Thanks for working on this! I have some general questions on the design, especially on the capability of customization. IMO we can focus on the API design for now and later integrate with other classes. Please let me know what you think.

This is fair, I just wanted to show the end to end picture for ease of understanding.

wgtmac · 2026-04-17T09:59:51Z

+# specific language governing permissions and limitations
+# under the License.
+
+iceberg_install_all_headers(iceberg/metrics)


Missing meson.build equivalent for this subdirectory

wgtmac

Follow-up findings from a deeper design/parity pass over the metrics reporting changes.

wgtmac · 2026-04-17T15:15:42Z

  committed_ = true;
  ctx_->table = std::move(commit_result.value());
-
+  ReportCommitMetrics();


Transaction::Commit() always calls ReportCommitMetrics() after a successful metadata commit, even for metadata-only transactions. That helper then reads current_snapshot() from the post-commit table and reports it as if this transaction created that snapshot, which can re-emit stale snapshot metrics and a misleading operation name. Java only reports commit metrics from snapshot-producing commits (CreateSnapshotEvent in SnapshotProducer), so the reporting hook should move closer to snapshot-producing updates or otherwise prove that this transaction actually created a new snapshot before emitting a CommitReport.

wgtmac · 2026-04-17T15:15:42Z


+  // Load metrics reporter from catalog properties
+  std::shared_ptr<MetricsReporter> reporter;
+  auto reporter_result = MetricsReporters::Load(final_config.configs());


The REST catalog now loads a local reporter from catalog properties, but it never constructs the built-in REST reporter for the /metrics endpoint and never combines the two. As a result, scan/commit reports from tables loaded through RestCatalog are never POSTed back to the server even when the server advertises ReportMetrics support. Java's RESTSessionCatalog explicitly combines the catalog reporter with RESTMetricsReporter, so this leaves the feature only partially implemented on the C++ side.

wgtmac · 2026-04-17T15:15:42Z

+/// \brief Infer the reporter type from properties.
+std::string InferReporterType(
+    const std::unordered_map<std::string, std::string>& properties) {
+  auto it = properties.find(std::string(kMetricsReporterImpl));


This loader treats metrics-reporter-impl as a lowercase registry key and defaults to noop, while Java and the Iceberg docs treat the same property as a fully qualified class name whose instance is initialized with catalog properties. On top of that, the catalog constructors ignore Load() errors and Table falls back to noop, so a Java-style config string or typo quietly disables metrics instead of surfacing a configuration error. That is a public-contract drift, not just an implementation detail.

while Java and the Iceberg docs treat the same property as a fully qualified class name whose instance is initialized with catalog properties

the divergence is because c++ does not support reflection. I decided on making things local case to reduce the risk of typos based on caps on/off.

typo quietly disables metrics instead of surfacing a configuration error
I will update to surface a configuration error.

wgtmac · 2026-04-17T15:15:42Z

+///
+/// Embedded in ScanReport and populated by DataTableScan after PlanFiles()
+/// completes. Mirrors the fields in Java's ScanMetricsResult.
+struct ICEBERG_EXPORT ScanMetrics {


ScanReport embeds a plain ScanMetrics struct of raw integers and std::chrono::nanoseconds, but Java's contract is layered: live counters/timers are collected in ScanMetrics, converted into ScanMetricsResult, and then wrapped by ScanReport. That layering preserves units, optional presence, noop semantics, and serialization behavior. In C++, every metric defaults to 0, so consumers cannot distinguish "not collected" from a real zero value, and the report shape is no longer compatible with Java's richer contract.

wgtmac · 2026-04-17T15:15:42Z

+  metric.total_equality_deletes = parse_int64(SnapshotSummaryFields::kTotalEqDeletes);
+  metric.added_dvs = parse_int64(SnapshotSummaryFields::kAddedDVs);
+  metric.removed_dvs = parse_int64(SnapshotSummaryFields::kRemovedDVs);
+  metric.created_manifest_count = parse_int64(SnapshotSummaryFields::kManifestsCreated);


CommitMetrics exposes replaced_manifest_count, but ReportCommitMetrics() never reads SnapshotSummaryFields::kManifestsReplaced into it. Java's CommitMetricsResult.from(...) does populate the corresponding manifestsReplaced field from the snapshot summary. Any consumer relying on this report will currently observe 0 even when the commit actually replaced manifests.

wgtmac · 2026-04-17T15:15:42Z

+    for (const auto& task : tasks) {
+      report.scan_metrics.total_file_size_in_bytes +=
+          task->data_file()->file_size_in_bytes;
+      for (const auto& del_file : task->delete_files()) {


The scan report counts equality and positional delete files, but it never increments indexed_delete_files and it never separates deletion vectors from ordinary positional deletes. Java's ScanMetricsUtil.indexedDeleteFile(...) increments all three counters (indexedDeleteFiles, positionalDeleteFiles/dvs, equalityDeleteFiles). Because the C++ report schema already exposes indexed_delete_files and dvs, leaving them unset produces parity drift and misleading metrics for DV-heavy tables.

wgtmac · 2026-04-17T15:15:42Z

+
+namespace iceberg {
+
+/// \brief Metrics collected during a table commit (snapshot creation).


CommitReport only carries summary-derived counters, but Java's commit reporting model also has live commit metrics (totalDuration, attempts) collected through CommitMetrics and serialized through CommitMetricsResult. Because those fields are absent from the C++ public model entirely, even a future implementation cannot fully mirror Java's commit report without another API change.

wgtmac · 2026-04-17T15:15:42Z

  /// \param io FileIO instance for reading manifests files.
+  /// \param reporter Optional metrics reporter for scan metrics.
+  /// \param table_name Optional table name for metrics reporting.
  static Result<std::unique_ptr<TableScanBuilder<ScanType>>> Make(


The builder stores a reporter internally, but there is no public scan-level API to add an extra reporter the way Java's Scan.metricsReporter(...) does. That means C++ can only use the reporter injected from the table/catalog path, while Java supports composing an additional reporter for a particular scan. For observability integrations and tests, that is a meaningful flexibility gap.

good catch will address.

wgtmac · 2026-04-17T15:34:37Z

I have a design question about the ScanReport / CommitReport model in this PR. What is the intended direction here with respect to the Java metrics API? In Java, the reporting model is layered around metrics collection/result types, which also carry unit information and are backed by reusable measurement utilities (for example counters/timers and the related conversion helpers). In this C++ version, the reports currently look more like raw result structs, and I do not yet see an equivalent measurement abstraction/tooling layer behind them. Is that divergence intentional for C++, or is this PR meant as an initial step toward a closer alignment with the Java design over time?

evindj · 2026-04-18T02:56:08Z

I have a design question about the ScanReport / CommitReport model in this PR. What is the intended direction here with respect to the Java metrics API? In Java, the reporting model is layered around metrics collection/result types, which also carry unit information and are backed by reusable measurement utilities (for example counters/timers and the related conversion helpers). In this C++ version, the reports currently look more like raw result structs, and I do not yet see an equivalent measurement abstraction/tooling layer behind them. Is that divergence intentional for C++, or is this PR meant as an initial step toward a closer alignment with the Java design over time?

The plan is still to keep parity as a next step I was thinking of doing serialization and then introduce the utilities. I initially elected to do it this way because I wanted to have an idea of the end to end view. Now I can probably update the first initial PR to just on the utilities if it will make your review easier I also don't want the change to be too big.

wgtmac · 2026-04-28T15:59:51Z

@evindj Sorry I've been busy these days. I think it would be good to focus on the API design first.

kamcheungting-db · 2026-05-04T05:49:11Z

  std::shared_ptr<FileIO> file_io_;
  std::string warehouse_location_;
  std::unique_ptr<class InMemoryNamespace> root_namespace_;
+  std::shared_ptr<MetricsReporter> reporter_;


please add comment about this new member

kamcheungting-db · 2026-05-04T06:04:19Z

+namespace {
+
+/// \brief Registry type for MetricsReporter factories.
+using MetricsReporterRegistry = std::unordered_map<std::string, MetricsReporterFactory>;


why not use enum as the key?

Do we have any use case that std::string fit better?

kamcheungting-db · 2026-05-04T06:15:41Z

  /// \param[in] metadata The metadata for the table.
  /// \param[in] metadata_location The location of the table metadata file.
  /// \param[in] io The FileIO to read and write table data and metadata files.
  /// \param[in] catalog The catalog that this table belongs to.


update comment of parameter list

kamcheungting-db · 2026-05-04T06:16:58Z

+    std::shared_ptr<TableMetadata> table_metadata, std::shared_ptr<FileIO> file_io,
+    std::shared_ptr<MetricsReporter> reporter, const std::string& table_name)
+    : metadata_(std::move(table_metadata)), io_(std::move(file_io)) {
+  context_.reporter = std::move(reporter);


should we also check port reporter like table.h above?

Just want to make all usage of reporter to be consistent

kamcheungting-db · 2026-05-04T06:21:05Z

+    report.scan_metrics.total_data_manifests =
+        static_cast<int64_t>(data_manifests.size());
+    report.scan_metrics.total_delete_manifests =
+        static_cast<int64_t>(delete_manifests.size());


if the source of the data are not int64_t, why do we declare them as int64_t ?
It looke like unnecessary type casting?

wgtmac · 2026-05-12T08:35:00Z

@evindj Do you have any chance to revive this?

evindj · 2026-05-12T13:46:07Z

@evindj Do you have any chance to revive this?

Sorry for the long wait, I have been a bit busy lately. My schedule should clear toward the end of the week.

wgtmac

I haven't finished my review yet. Just post my findings so far and I'll finish it in two days. I think this change looks great. Thank you for polishing the design!

wgtmac · 2026-05-27T09:01:34Z

+  }
+
+  /// \brief Return a shared no-op timer singleton.
+  static Timer& Noop();


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

wgtmac · 2026-05-27T09:07:28Z

+  virtual bool IsNoop() const { return false; }
+
+  /// \brief Return a shared no-op counter singleton.
+  static Counter& Noop();


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

wgtmac · 2026-05-27T09:08:53Z

+  virtual void Increment(int64_t amount) = 0;
+
+  /// \brief Return the current count.
+  virtual int64_t Value() const = 0;


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

wgtmac · 2026-05-27T09:09:44Z

+  virtual ~Counter() = default;
+
+  /// \brief Increment the counter by 1.
+  virtual void Increment() = 0;


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

wgtmac · 2026-05-27T09:12:48Z

+  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;
+    }
+  }


Suggested change

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;

}

}

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.

wgtmac · 2026-05-27T09:16:39Z

+class ICEBERG_EXPORT ScanMetrics {
+ public:
+  /// \brief Create a ScanMetrics instance backed by the given MetricsContext.
+  static ScanMetrics Of(MetricsContext& context);


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

Do we want to make the default ctor private?

wgtmac · 2026-05-27T09:44:32Z

+
+ScanMetrics ScanMetrics::Of(MetricsContext& context) {
+  ScanMetrics m;
+  m.total_planning_duration = context.GetTimer("totalPlanningDuration");


Should it be total-planning-duration?

Same for all below. CommitMetrics are aligned.

wgtmac · 2026-05-27T09:45:51Z

+#include "iceberg/metrics/scan_report.h"
+#include "iceberg/result.h"
+
+namespace iceberg {


Rename this file to json_serde_internal.h to not install it by accident.

wgtmac

I've finished reviewing on non-test files. Great work! Let me know what you think. Thanks!

wgtmac · 2026-05-28T02:48:06Z

+
+/// \brief MetricsContext backed by DefaultCounter and DefaultTimer instances.
+///
+/// Thread-safe for metric *increments*; the unordered_map lookup/insert is NOT


Should we use a mutex to make it thread safe? It should not hurt too much.

wgtmac · 2026-05-28T02:49:47Z

+  std::shared_ptr<Timer> GetTimer(std::string_view name) override;
+
+ private:
+  std::unordered_map<std::string, std::shared_ptr<Counter>> counters_;


Should we use StringHash and StringEqual defined in string_util.h to enable heterougenous lookup to avoid repeated string allocation?

wgtmac · 2026-05-28T02:52:07Z

+  /// \brief Return the null (no-op) MetricsContext singleton.
+  ///
+  /// All metrics returned by the null context are noop; nothing is allocated.
+  static MetricsContext& Null();


Should we change the return type to std::shared_ptr<MetricsContext>? It really depends on the downstream use case. It will not be helpful if any place that uses it expects a shared_ptr.

wgtmac · 2026-05-28T02:53:25Z

+  /// Called by MetricsReporters::Load() before the first Report() invocation.
+  /// The default implementation is a no-op. Override to perform property-based
+  /// setup (e.g., configure endpoints, credentials, sampling rates).
+  virtual void Initialize(


Do we want to return Status for both Initialize and Report just in case a report impl will actually return any error?

wgtmac · 2026-05-28T03:04:17Z

+  const std::unordered_set<std::shared_ptr<MetricsReporter>>& Reporters() const;
+
+ private:
+  std::unordered_set<std::shared_ptr<MetricsReporter>> reporters_;


Usually we won't have too much reporters. Perhaps std::vector is sufficient and more performant

wgtmac · 2026-05-28T03:05:57Z

+/// \brief A MetricsReporter that delegates to multiple reporters.
+///
+/// Combines several reporters so that every report is delivered to each of them.
+/// Any exception thrown by an individual reporter is caught and swallowed;


Should we make it a contract that a MetricsReporter should not throw but return an error status instead?

wgtmac · 2026-05-28T03:20:20Z

+
+  /// \brief Register a factory for a metrics reporter type.
+  ///
+  /// This method is not thread-safe. All registrations should be done during


How about making it thread safe?

evindj marked this pull request as draft March 11, 2026 20:33

evindj force-pushed the metrics_core_components branch from 5fa02cb to 0a85180 Compare March 11, 2026 20:48

evindj force-pushed the metrics_core_components branch 2 times, most recently from 5241044 to d26fb96 Compare March 25, 2026 03:15

evindj marked this pull request as ready for review March 25, 2026 04:08

wgtmac reviewed Mar 31, 2026

View reviewed changes

evindj marked this pull request as draft March 31, 2026 13:59

evindj marked this pull request as ready for review April 2, 2026 05:08

wgtmac reviewed Apr 17, 2026

View reviewed changes

kamcheungting-db reviewed May 4, 2026

View reviewed changes

evindj force-pushed the metrics_core_components branch 3 times, most recently from 56026ae to b5156af Compare May 20, 2026 21:27

feat: metrics reporting for scan and commit

dbf02c0

evindj force-pushed the metrics_core_components branch from f787939 to dbf02c0 Compare May 21, 2026 18:24

cleanup

9c89310

cleanup

b87f96a

wgtmac reviewed May 27, 2026

View reviewed changes

wgtmac reviewed May 28, 2026

View reviewed changes

wgtmac added the awaiting author action Author needs to address comments, resolve conflicts, answer questions, etc. label May 28, 2026


		namespace iceberg {

		/// \brief Metrics collected during a table commit (snapshot creation).

Conversation

evindj commented Mar 11, 2026

Uh oh!

wgtmac commented Mar 20, 2026

Uh oh!

evindj commented Mar 21, 2026

Uh oh!

wgtmac commented Mar 27, 2026

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evindj Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evindj commented Mar 31, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgtmac commented Apr 17, 2026

Uh oh!

evindj commented Apr 18, 2026

Uh oh!

wgtmac commented Apr 28, 2026

Uh oh!

kamcheungting-db May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

evindj Mar 31, 2026 •

edited

Loading

kamcheungting-db May 4, 2026 •

edited

Loading