moomindani commented on PR #16574: URL: https://github.com/apache/iceberg/pull/16574#issuecomment-4554696474
Hi @gaborkaszab, Thanks for the catch — that was unintended. The filter is meant to apply uniformly, and the asymmetry between the `CatalogUtil`-wrapped user reporter and the catalog-injected `RESTMetricsReporter` was a real bug. Pushed a follow-up commit: - **Fix**: `RESTSessionCatalog.metricsReporter(...)` now wraps the `RESTMetricsReporter` with the same `FilteringMetricsReporter` before combining with the user reporter, so both sides of the combined reporter honor the filter. Went with the local wrap in `RESTSessionCatalog` over a couple of alternative shapes (combine-aware filter, scan/commit-framework wrap) — added a short rationale section to the PR description. - **REST catalog test** (`TestRESTCatalog.metricsFilterAppliesToRestMetricsReporter`): exercises `RESTSessionCatalog.metricsReporter(...)` with filter properties and a mock `RESTClient`; verifies that one filtered + one unfiltered report produce exactly one `post()` (the filtered one short-circuits before reaching the client). - **Configs-driven catalog test** (`TestFilteringMetricsReporter.loadMetricsReporterFiltersThroughUserConfiguredReporter`): sets up `metrics-reporter-impl` + filter properties through `CatalogUtil.loadMetricsReporter` with a static-singleton capturing reporter, demonstrating the wrap actually applies at the catalog wiring level — that's the "general catalog test" you asked about. I also locally combined this PR with the proposed OTel `MetricsReporter` from #16250 and ran an integration test against `InMemoryMetricReader` to verify the composition. Excluded tables don't reach the OTel pipeline; included tables emit normally. The two layers (table-name filter + OTel attribute allowlist) compose cleanly via the `MetricsReporter` interface without special wiring. Not committed here since #16250 hasn't landed. Fully agree on the community buy-in point — the cardinality concern that motivated this proposal came up in the dev@ DISCUSS for #16250 (the OTel reporter, which is where Grant flagged it), not on a thread specific to this PR. Happy to wait for broader signal. Just wanted the design question and the tests to be in a reviewable state for when reviewers come back. Thanks again for the careful read. -- 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]
