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-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

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

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: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

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

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

Comment thread src/iceberg/metrics/CMakeLists.txt

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 4 times, most recently from f787939 to dbf02c0 Compare May 21, 2026 18:24

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

evindj force-pushed the metrics_core_components branch from b87f96a to b87c145 Compare May 28, 2026 23:34

evindj requested a review from wgtmac May 29, 2026 21:28

Innocent and others added 3 commits June 5, 2026 17:38

feat: metrics reporting for scan and commit

e80f120

ubuntu build fix

5f8895e

align with java impl & spec

f600898

wgtmac force-pushed the metrics_core_components branch from 64c2353 to f600898 Compare June 5, 2026 09:44

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evindj commented Mar 31, 2026

Uh oh!

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

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 May 12, 2026

Uh oh!

evindj commented May 12, 2026

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

kamcheungting-db May 4, 2026 •

edited

Loading