Core: Add table-name filter for MetricsReporter - #16574
Conversation
gaborkaszab
left a comment
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.
|
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. |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
425acd7 to
132b48e
Compare
|
Not stale — this PR is actively maintained and waiting for reviewer feedback. Rebased onto latest The design questions raised in the last review round have been addressed (REST-reporter symmetry fix + general catalog test). Happy to incorporate any further feedback whenever a reviewer has a chance to take a look. |
|
Still active and waiting for review — not stale. Status since the last update:
@gaborkaszab would you mind taking another look at the REST fix, since you raised the original issue? @ebyhr @jbonofre — a review from any of you would also be welcome. This is a small, self-contained change on top of #16573. |
|
Hi @moomindani , Would be nice to hear the opinion of experts on this, but here is why I'm somewhat hesitant on the approach:
Before moving forward I'd suggest having a wider community opinion on this. |
|
Thanks @gaborkaszab — both concerns are fair, and the second one is a real limitation of the current shape rather than something I'd argue away. Below are the options I see for each, so the discussion has something concrete to react to. I'll take this to dev@ as you suggest. Concern 1 — Is the table name the right predicate? Regex or exact match?
Detail on the tradeoffs:
My read: I lean A or C, without strong conviction. This is the part I'd most like broader input on, since it's a question about how operators actually organize tables, not about the implementation. Concern 2 — Catalog properties require a restartConfirmed, and to be precise about the scope: this is not specific to this PR.
Detail on the tradeoffs:
My read: B is out of scope here, so the real choice is A vs. C — which is ultimately a question of how much this feature is worth if it can only be set at startup. Stepping back — could this be left to reporter implementations and backend tooling?Worth naming explicitly as a "don't do this" option: ship no built-in filter, and let each reporter or the backend handle it. I looked into how OpenTelemetry expects this to be solved, since that was the original context in #16250, and the answer is narrower than I assumed:
So "leave it to backend tooling" is a real option, but it's specifically an option to support OTel-with-a-Collector users and not the others. That tradeoff seems worth being explicit about rather than assuming the backend can always absorb it. Next stepI'll start a [DISCUSS] thread on dev@ with this summary and link it here. Happy to hold the PR until there's a direction, and to rework it toward whichever shape the community prefers — including dropping it if the consensus is that this doesn't belong in the framework. |
|
Started the dev@ [DISCUSS] thread as promised: https://lists.apache.org/thread/dmf60fs64swkcy8cpdgkxn2nldx836xo It covers both of your concerns with the alternatives laid out, plus the "should this be in the framework at all" question. Holding this PR until there's a direction from the list. |
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
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.
The rebase onto main picked up the change that makes RESTMetricsReporter export asynchronously via a single-threaded executor. The filter test verified post() synchronously, so it raced the background export and failed intermittently in CI (zero interactions). Use timeout()-based verification, matching the existing async CommitReport tests.
…lter The table-name include / exclude properties took a single regex, so covering several databases meant hand-writing an alternation. Accept a comma-separated list instead, matching how the same problem is configured elsewhere: Debezium's table.include.list, DataHub's table_pattern allow/deny, and OpenMetadata's tableFilterPattern all take a list of expressions. Patterns were already matched against the whole name via Matcher.matches(), which is the same choice Debezium documents as an "anchored regular expression". That property is worth stating explicitly, since an unanchored prod.* would otherwise capture production.orders and prod_sandbox.orders -- a silent over-match that is hard to notice. Documented it and added a test that pins the behavior.
Filtering only by table name relies on a naming convention: a pattern written today silently captures tables created tomorrow, and the failure mode is invisible because metrics simply stop appearing for a table nobody thought about. A namespace is part of a table's identity instead, so a table added to an included namespace later is picked up automatically and a table outside it cannot match by accident. Add metrics-reporter.namespace.include / .exclude alongside the existing table-name filters. Both levels are applied independently, so a namespace can be selected while individual noisy tables inside it are still excluded by name -- the coarse-plus-fine combination that database.include.list plus table.include.list provides in Debezium, and schema_pattern plus table_pattern in DataHub. The namespace is derived from the reported table name by removing the catalog name, which the catalog supplies when it loads the reporter. Knowing the catalog name is what makes this unambiguous: the reported name is built by CatalogUtil#fullTableName, which switches its separator for URI-like catalog names and offers no way to tell a dotted catalog name from the namespace that follows it. Tests cover both shapes, along with multi-level and empty namespaces. Configuring a namespace filter where no catalog name is available fails at initialization rather than silently dropping every report.
c706ebc to
5da80bf
Compare
|
@gaborkaszab I've reworked the PR to address both of your concerns. Rather than argue for the original shape, I went looking for how other projects solve the same problem, since the dev@ thread [1] hasn't drawn input — that seems to be the norm for the list rather than a signal about this proposal, as eight of the twelve [DISCUSS] threads started last month have no replies. Three projects that filter tables for exactly this reason converge on the same design:
Two things stood out, and both are now in the PR. 1. Both levels, not one instead of the other. All three offer a coarse level and a per-table level. That matches my own operational experience: selecting whole databases is the common case, but production configuration eventually needs to exclude a specific table inside an otherwise interesting database, and a namespace-only filter cannot express that. So the PR keeps table-name filtering and adds This directly addresses your point about name-based prediction being fragile. A namespace is part of a table's identity rather than a naming convention, so a table added to an included namespace later is picked up automatically, and a table outside it cannot match by accident. Namespace filtering is the safer default; the table-name level remains for what a namespace cannot express. 2. A list of patterns, not a single one. All three accept a list. The PR now does too, so covering several databases no longer means hand-writing an alternation. On the surprise-capture risk specifically, it's worth stating something that was already true but undocumented: patterns are matched against the entire name via Deriving the namespace does not touch On your second concern (static lifecycle). I haven't changed this, and I still think the honest framing is the one from my earlier comment: adding config reload would mean inventing a mechanism Iceberg doesn't have, which shouldn't be settled as a side effect of this feature. What did change is that namespace filtering reduces how often you'd need to touch the config at all — adding a table to an included namespace requires no config change, and therefore no restart. That doesn't eliminate the limitation you identified, but it makes it bite less often in the case that motivated this. Two commits, split for review: one for the pattern list, one for the namespace filter. All 39 CI checks are green. Happy to keep iterating on the shape. [1] https://lists.apache.org/thread/dmf60fs64swkcy8cpdgkxn2nldx836xo |
|
One more data point for why filtering before the reporter matters, which I hadn't been able to quantify earlier. The OpenTelemetry metrics SDK applies a cardinality limit per metric stream, defaulting to 2000 attribute combinations. Beyond it, the SDK doesn't drop measurements — it folds them into one data point marked That puts a number on the "per-table cardinality" concern from the #16250 dev@ thread: with a table-name attribute in play, more than ~2000 distinct tables is enough to lose the breakdown. Filtering at the framework layer addresses it at the source — the reports never become time series in the first place — whereas the SDK-side options are to raise the limit or drop the attribute wholesale, and the Collector-side option only exists for OTel users. To be clear about scope: this is an argument for filtering existing somewhere, not for any particular shape of it. Your two concerns are still the open questions, and I'm happy to follow your read on Q1. |
2 participants
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.