Copilot commented on code in PR #701:
URL: https://github.com/apache/iceberg-cpp/pull/701#discussion_r3437267773
##########
src/iceberg/table_scan.cc:
##########
@@ -522,13 +540,22 @@ Result<std::vector<std::shared_ptr<FileScanTask>>>
DataTableScan::PlanFiles() co
return std::vector<std::shared_ptr<FileScanTask>>{};
}
+ auto metrics_context = MetricsContext::Default();
+ std::shared_ptr<ScanMetrics> scan_metrics =
ScanMetrics::Make(*metrics_context);
+ auto timed = scan_metrics->total_planning_duration->Start();
Review Comment:
`ScanMetrics::Make()` returns `std::unique_ptr<ScanMetrics>` (see
`metrics/scan_report.h`), so assigning it directly to
`std::shared_ptr<ScanMetrics>` will not compile. Construct the shared_ptr from
the returned unique_ptr (same pattern used elsewhere, e.g. catalog reporter
loading).
##########
src/iceberg/test/fast_append_test.cc:
##########
@@ -199,4 +205,151 @@ TEST_F(FastAppendTest, SetSnapshotProperty) {
EXPECT_EQ(snapshot->summary.at("custom-property"), "custom-value");
}
+// ---------------------------------------------------------------------------
+// Metrics integration tests
+// ---------------------------------------------------------------------------
+
+namespace {
+
+class CapturingReporter final : public MetricsReporter {
+ public:
+ Status Report(const MetricsReport& report) override {
+ reports_.push_back(report);
+ return {};
+ }
+ const std::vector<MetricsReport>& reports() const { return reports_; }
+ void clear() { reports_.clear(); }
+
+ private:
+ std::vector<MetricsReport> reports_;
+};
+
+CapturingReporter* g_capturing_reporter = nullptr;
+
+void RegisterCapturingReporter() {
+ static std::once_flag flag;
+ std::call_once(flag, [] {
+ (void)MetricsReporters::Register(
+ "fast.append.test.reporter",
+ [](const auto&) -> Result<std::unique_ptr<MetricsReporter>> {
+ auto ptr = std::make_unique<CapturingReporter>();
+ g_capturing_reporter = ptr.get();
+ return ptr;
+ });
Review Comment:
This test stores a raw pointer to a reporter instance that is owned by a
`std::unique_ptr` returned from the registry factory. After the catalog/table
is destroyed, `g_capturing_reporter` becomes dangling, which can cause
cross-test flakiness or accidental use-after-free if accessed later. Prefer
capturing reports via static storage owned by the test (e.g., a static
mutex+vector inside `CapturingReporter`) or reset the global pointer during
fixture teardown at minimum.
##########
src/iceberg/catalog/rest/rest_metrics_reporter.cc:
##########
@@ -0,0 +1,69 @@
+/*
+ * 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/catalog/rest/rest_metrics_reporter.h"
+
+#include <cstdio>
+#include <utility>
+
+#include <nlohmann/json.hpp>
+
+#include "iceberg/catalog/rest/auth/auth_session.h"
+#include "iceberg/catalog/rest/error_handlers.h"
+#include "iceberg/catalog/rest/http_client.h"
+#include "iceberg/metrics/json_serde_internal.h"
+#include "iceberg/metrics/metrics_reporter.h"
+
+namespace iceberg::rest {
+
+namespace {
+
+constexpr std::string_view kReportType = "report-type";
+constexpr std::string_view kScanReportType = "scan-report";
+constexpr std::string_view kCommitReportType = "commit-report";
+
+} // namespace
+
+RestMetricsReporter::RestMetricsReporter(std::shared_ptr<HttpClient> client,
+ std::string metrics_endpoint,
+ std::shared_ptr<auth::AuthSession>
session)
+ : client_(std::move(client)),
+ metrics_endpoint_(std::move(metrics_endpoint)),
+ session_(std::move(session)) {}
+
+Status RestMetricsReporter::Report(const MetricsReport& report) {
+ // Serialize the report variant to JSON.
+ Result<nlohmann::json> json_result = std::visit(
+ [](const auto& r) -> Result<nlohmann::json> { return ToJson(r); },
report);
+ if (!json_result) {
+ return {};
+ }
+
+ // Inject "report-type" required by the REST spec (not included in core
ToJson).
+ auto& json = json_result.value();
+ json[kReportType] =
+ std::holds_alternative<ScanReport>(report) ? kScanReportType :
kCommitReportType;
+
+ // POST to the metrics endpoint; suppress errors to match Java
fire-and-forget behavior.
+ std::ignore = client_->Post(metrics_endpoint_, json.dump(), /*headers=*/{},
+ *DefaultErrorHandler::Instance(), *session_);
+ return {};
+}
Review Comment:
`MetricsReporter` implementations are required to be non-throwing, but
`nlohmann::json` operations (including `dump()`) can throw exceptions. Without
a catch-all here, an exception would violate the interface contract and could
unwind through commit/scan code paths. Wrap the body in a `try { ... } catch
(...) {}` (still returning OK to keep the fire-and-forget behavior).
--
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]
