Core: Add table-name filter for MetricsReporter#16574
Conversation
Add an optional filtering layer above any MetricsReporter implementation that drops ScanReports and CommitReports whose tableName() does not pass the configured include / exclude regex. Two new catalog properties control the filter: metrics-reporter.table-name.include and metrics-reporter.table-name.exclude. Both are Java regex patterns matched against the table name; when both are set, exclude wins over include. When neither property is set, CatalogUtil.loadMetricsReporter returns the underlying reporter unchanged, so the default code path incurs no runtime overhead. Empty values are treated as not set to avoid accidentally silencing all metrics on misconfiguration. Invalid regex values fail fast at catalog initialization with a clear error pointing at the offending property. The filter applies uniformly across all reporter implementations (LoggingMetricsReporter, RESTMetricsReporter, and custom user-supplied ones). Reports whose subtype does not expose a table name are forwarded without filtering. Closes apache#16573
gaborkaszab
left a comment
There was a problem hiding this comment.
Hey @moomindani ,
I'm not sure this works with REST catalog. It creates a reporter using CatalogUtil.loadMetricsReporter where you wrap it with FilteringMetricsReporter, but then for each table it combines this with a RESTMetricsReporter that is not filtering. Is this the intended design? Would be nice to see some REST catalog tests too. Maybe some general catalog test where we set up the MetricsReporter via configs, and then it gets wrapped with the filtering one?
Before going deeper into the code, I'd wait to see if there is community buy in for this change, TBH.
The table-name filter introduced earlier in this PR is applied via CatalogUtil.loadMetricsReporter to the user-configured reporter, but RESTSessionCatalog injects an additional RESTMetricsReporter per table inside metricsReporter(...), which previously bypassed the filter. Wrap that RESTMetricsReporter with the same FilteringMetricsReporter (using the catalog properties stored at init) before combining with the user reporter, so both sides of the combined reporter honor the configured table-name filter. Add two tests: - TestRESTCatalog.metricsFilterAppliesToRestMetricsReporter exercises RESTSessionCatalog.metricsReporter(...) with filter properties and a mock RESTClient, verifying that one filtered + one unfiltered scan report produce exactly one post() and that the filtered report short-circuits before reaching the client. - TestFilteringMetricsReporter.loadMetricsReporterFiltersThroughUserConfiguredReporter goes through CatalogUtil.loadMetricsReporter with a static-singleton capturing reporter, demonstrating that the wrap applies at the catalog wiring level when metrics-reporter-impl plus the filter properties are configured together.
|
Hi @gaborkaszab, Thanks for the catch — that was unintended. The filter is meant to apply uniformly, and the asymmetry between the Pushed a follow-up commit:
I also locally combined this PR with the proposed OTel 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. |
Closes #16573.
Adds an optional filtering layer above any
MetricsReporterimplementation that dropsScanReportandCommitReportinstances whosetableName()does not pass the configured include / exclude regex. The filter applies uniformly toLoggingMetricsReporter,RESTMetricsReporter, and custom user-supplied reporters. The proposal surfaced in the dev@ DISCUSS thread for #16250 (per-table cardinality of the OTel reporter) and is intentionally scoped as cross-reporter, not OTel-specific.Design
CatalogUtil.loadMetricsReporterwraps the resolved reporter in aFilteringMetricsReporterwhen either of the new properties is set. When neither is set, the resolved reporter is returned unchanged — no wrapper instantiated, no runtime overhead on the default path.MetricsReportsubtypes that do not expose a table name (anything other thanScanReport/CommitReport) are forwarded without filtering.Configuration
Two new catalog properties:
Values are Java regex patterns matched against the table name. When both are set,
excludewins overinclude(an explicit deny overrides an include). Empty values are treated as not set to avoid accidentally silencing all metrics on misconfiguration. Invalid regex values fail fast at catalog initialization with a clear error pointing at the offending property.Behavior:
includeonly: forward reports whose table name matches; drop others.excludeonly: drop reports whose table name matches; forward others.excludematches; otherwise forward only ifincludematches.This mirrors the existing
route-regexpattern used iniceberg-kafka-connect(IcebergSinkConfig), where a user-supplied regex from configuration is compiled viaPattern.compile()and matched against incoming data. Same trust model: catalog property = admin-controlled.Design choice — where the filter wrap lives in REST catalog flow
The REST catalog adds an additional
RESTMetricsReporterper-table insideRESTSessionCatalog.metricsReporter(...), separate from the user'smetrics-reporter-impl. To make the table-name filter apply uniformly to both reporters, three shapes were considered:A. Wrap inside
RESTSessionCatalog.metricsReporter(...)(chosen). TheRESTMetricsReporteris wrapped withFilteringMetricsReporterusing the catalog properties stored at init, then combined with the user reporter. Smallest local change. KeepsMetricsReportersandFilteringMetricsReportermutually unaware. The additional wrap lives next to the existingcombine(reporter, restMetricsReporter)line, which is itself REST-specific wiring.B. Make
MetricsReporters.combine()aware ofFilteringMetricsReporter. Detect when one input is a filtering wrapper and "lift" it to wrap the composite. Avoids touching REST catalog code; would apply uniformly to any future combine site. But couples a generic utility to a specific reporter implementation and changescombine()semantics for all callers.C. Wrap at the scan/commit framework layer (
TableScanContext/BaseTable.combineMetricsReporter). Apply the filter wrap on the final composed reporter at the point it's used. Catalog-agnostic. But touches scan/commit framework code for what is logically a catalog-level concern, and requires plumbing filter properties down to that layer.Option A was preferred because the
combine(reporter, restMetricsReporter)line inRESTSessionCatalogis already REST-specific wiring (no other catalog does this combine), so the additional wrap lives in the same conceptual location rather than introducing knowledge of filtering elsewhere. Happy to revisit if reviewers prefer one of the other shapes.Disclosure
Per the project's AI-assisted contribution guidelines, I used Claude Code to help draft this work. I reviewed every change by hand and ran the full test/lint loop locally before opening this PR. The design and motivation discussion is in #16573.
cc @ebyhr @jbonofre — happy to address any feedback.