moomindani commented on PR #16574: URL: https://github.com/apache/iceberg/pull/16574#issuecomment-5089662824
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? | | Option | Expresses #16573's use cases | Surprise-capture risk | Config burden | |---|---|---|---|---| | **A** | Table-name regex *(current PR)* | Yes — `prod.*`, exclude `tmp.*` | **Yes** — a pattern written today silently captures tables created tomorrow | Low (one line) | | **B** | Exact-match name list | No — requires enumerating every table | None | High — every new table is a config change | | **C** | Namespace-level filter | Mostly — most cases are "this whole database" | Low — namespace is structural, not a naming convention | Low | | **D** | Exact list + optional regex | Yes, if you opt in | Yes, but as an explicit choice | Low–High (two mechanisms) | **Detail on the tradeoffs:** - **A** matches how the same problem is already solved in this repo — `iceberg-kafka-connect` compiles a user-supplied `route-regex` from config (`IcebergSinkConfig`). Its failure mode is the one you name, and it's invisible: metrics just stop appearing for a table nobody thought about. - **B** is the safest and most auditable, but it doesn't express the motivating cases. "Report only production tables" across thousands of tables means maintaining thousands of names — and, given Concern 2, a catalog restart per addition. It turns a one-line policy into an operational chore. - **C** is worth considering because most cases in #16573 are namespace-shaped rather than table-shaped. Caveat: `ScanReport`/`CommitReport` expose only a flat `tableName()` string today, so this means parsing the name back into an identifier. It also can't express intra-namespace exclusions (a noisy scratch table inside an otherwise interesting database). **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 restart Confirmed, and to be precise about the scope: this is not specific to this PR. `metrics-reporter-impl` and every other catalog property share the same lifecycle. The patterns compile once at catalog initialization and live as long as the catalog instance, so an engine holding a single long-lived catalog connector can only change this by restarting. | | Option | Runtime-adjustable | Scope of change | |---|---|---|---| | **A** | Accept static lifecycle *(current PR)* | No | None — consistent with all existing properties | | **B** | Periodic / signal-based reload | Yes | **Large** — Iceberg has no mechanism for re-reading catalog properties | | **C** | Leave dynamic control to the reporter impl | Yes, user-implemented | None — `MetricsReporter.initialize(Map)` already exists | **Detail on the tradeoffs:** - **A** is the rigidity you point at. For long-lived-catalog engines, tuning what gets reported becomes a restart-level operation — which in practice means nobody tunes it. - **B** would require inventing a config-reload facility (reporter-local refresh loop, or something broader). That's a much bigger design conversation, and it shouldn't be settled as a side effect of table filtering. - **C** narrows this PR's value honestly: users needing runtime control implement it in their own reporter against whatever config source they already operate, and this PR covers the common static case without anyone writing code. Worth noting that for the OTel reporter specifically, operators who run a Collector already have a runtime-adjustable path — the Collector's `filterprocessor` applies attribute-value predicates centrally, without restarting anything on the Iceberg side. That doesn't help `LoggingMetricsReporter` or `RESTMetricsReporter` users, but it does mean the static limitation bites unevenly depending on the reporter. **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: - **SDK Views cannot do this.** Per the [metrics SDK spec](https://opentelemetry.io/docs/specs/otel/metrics/sdk/), instrument selection predicates are `name` / `type` / `unit` / `meter_name` / `meter_version` / `meter_schema_url` — attribute values are not selection criteria. `attribute_keys` is an allow/exclude list of attribute *keys*, not values. So "drop the streams where `iceberg.table.name` matches `tmp.*`" is not expressible as a View. This is the same distinction as the `iceberg.otel.metrics.attributes` allowlist in #16250: that controls which attributes a metric carries, not whether the metric is emitted at all. - **Value-based filtering lives in two places in the OTel model:** the Collector (`filterprocessor`), or at instrumentation time before recording. This PR is the second one, so filtering in the reporter layer isn't a deviation from how OTel expects this to work. - **The Collector path only covers OTel users.** `LoggingMetricsReporter` and `RESTMetricsReporter` have no equivalent, and a per-reporter solution means each one re-solves it slightly differently — which was the original argument in #16573 for putting it above the reporter layer. 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 step I'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. -- 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]
