refactor(metrics): move IcebergMetricsReporter SPI to polaris-core - #5204
Open
flyingImer wants to merge 6 commits into
Open
refactor(metrics): move IcebergMetricsReporter SPI to polaris-core#5204flyingImer wants to merge 6 commits into
flyingImer wants to merge 6 commits into
Conversation
Collaborator
Author
|
@obelix74 ^^^ |
adutra
reviewed
Aug 3, 2026
flyingImer
force-pushed
the
fix/metrics-spi-core-placement
branch
from
August 5, 2026 22:45
d2ce602 to
14db01a
Compare
Collaborator
Author
|
Thanks @adutra incorporated your comments! |
dimas-b
reviewed
Aug 6, 2026
PR apache#5068 put this SPI (plus its MetricsReportEnvelope carrier and MetricType discriminator) in runtime/service. As raised in review (apache#5068 (comment)), the interface has no CDI, Quarkus or JAX-RS dependency at all: only guava, jspecify, and Iceberg's own types. Nothing in the type stops it being implemented outside Quarkus; only the module we put it in did. Introduces a shared spi module for framework-free contracts and lands these three types in it. The reporter is a substrate contract rather than a feature one: the user-facing operation is the Iceberg REST catalog's own metrics endpoint, gated by the ordinary table data privileges, and the reporter is the replaceable sink that endpoint hands the report to. That puts it alongside authorization in the call graph, which is why it sits under spi.substrate rather than spi.feature. Only the metrics contracts move here. Relocating anything else out of polaris-core is deliberately left out of this change.
…reports/base Now that the IcebergMetricsReporter SPI lives in its own module, extensions/metrics-reports/base no longer needs runtime/service for anything, and the default logging implementation can live alongside its peers (NoOpMetricsReporter, and PersistingMetricsReporter in the relational-jdbc extension) instead of in the Quarkus runtime module. Adds guava to the module's own dependencies, for the @VisibleForTesting that LoggingMetricsReporter carries on its test constructor. It reaches this module only through a transitive implementation configuration today, which doesn't put it on a consumer's compile classpath. Same pattern as extensions/auth/opa and extensions/semantic-models.
Import-only changes to point every remaining consumer at
org.apache.polaris.spi.substrate.metrics: NoOpMetricsReporter,
PersistingMetricsReporter, IcebergCatalogHandler,
IcebergCatalogHandlerFactory, ServiceProducers, and
IcebergCatalogHandlerTest. None of these change behavior; selection
logic in ServiceProducers.metricsReporter(...) is untouched.
extensions/metrics-reports/persistence/relational-jdbc no longer needs
runtime/service either, for the same reason as the base module. Both
extensions and runtime/service now take a dependency on the spi module
for the contract. runtime/service needs it on testFixtures too, because
TestServices supplies a lambda reporter to the handler builder, so the
interface has to resolve there as well.
runtime/service previously carried its default reporter
(LoggingMetricsReporter) directly, so it never needed to depend on the
metrics extension modules. Now that the default lives in
extensions/metrics-reports/base, add it back as runtimeOnly: the
packaged Quarkus app needs it on its runtime classpath for bean
discovery, same as the existing runtimeOnly(":polaris-relational-jdbc")
line right above it. runtime/server already carries this same
dependency for the assembled distribution.
quarkus.log.category."org.apache.polaris.service.reporting".level=OFF targets a package that only ever held MetricsReportingConfiguration (a config interface, no logger), so it silences nothing. The actual reporter logs under LoggingMetricsReporter, whose class-level javadoc already documents its logger category as org.apache.polaris.extension.metrics.reports. Point the OFF setting there instead, so the shipped default matches what the reporter documents about itself. Pre-existing bug, uncovered by the SPI move in the two preceding commits, not caused by it.
Now that IcebergMetricsReporter, LoggingMetricsReporter, and NoOpMetricsReporter are all peers in extensions/metrics-reports/base, "which one runs when nobody configures anything" is a choice that can be made on its own merits rather than following from which impl happened to already live in runtime/service. Swap which class owns the @Identifier("default") value: NoOpMetricsReporter takes it, LoggingMetricsReporter moves to @Identifier("logging"). polaris.iceberg-metrics.reporting.type's own default value, and the application.properties line that sets it, are both left as "default". That string stays a stable, always-valid selection; it now just resolves to the quiet choice instead of the logging one. Deployments that already set the property explicitly (to "default", "logging", or "persisting") are unaffected either way. Verified against org.apache.polaris.service.it.RestCatalogFileIT: testSendMetricsReport still returns 204 with no reporter configured, and (checking the raw test log) no longer logs the report, confirming the no-op path is actually selected and the request still succeeds.
Before this line of commits, LoggingMetricsReporter was the ambient default: every deployment that configured nothing got it, so silencing its logger by default (via a log-category override, on top of already selecting it) protected against every unconfigured install being noisy out of the box. Now that a no-op reporter is the ambient default, reaching LoggingMetricsReporter at all requires deliberately setting polaris.iceberg-metrics.reporting.type=logging. At that point the original rationale for a second, separate opt-in (bumping a log category to INFO) no longer applies: nobody reaches this reporter without already having made one explicit choice. This also brings it in line with its sibling opt-in reporter, PersistingMetricsReporter, which performs its own purpose (persisting) immediately upon selection with no equivalent secondary gate. Removes the quarkus.log.category."org.apache.polaris.extension.metrics.reports".level=OFF override, so selecting "logging" now logs at INFO right away, matching the root log level like any other unconfigured logger. Updates LoggingMetricsReporter's javadoc: it previously promised logging was disabled by default, a promise the override existed to keep and that no longer applies once the override is gone.
flyingImer
force-pushed
the
fix/metrics-spi-core-placement
branch
from
August 12, 2026 22:07
14db01a to
0c78d66
Compare
dimas-b
reviewed
Aug 12, 2026
dimas-b
left a comment
dimas-b
left a comment
Contributor
There was a problem hiding this comment.
Continuing the discussion from the online meeting. I hope my line of comments is consistent with what I was saying in the call 😅 I seek to clarify my stance with these comments.
| # Configuration for the behaviour of the metrics endpoint | ||
| polaris.iceberg-metrics.reporting.type=default | ||
| # Set to INFO if you want to see iceberg metric reports logged | ||
| quarkus.log.category."org.apache.polaris.service.reporting".level=OFF |
| * under the License. | ||
| */ | ||
| package org.apache.polaris.service.metrics; | ||
| package org.apache.polaris.spi.substrate.metrics; |
Contributor
There was a problem hiding this comment.
What is substrate?
Why not org.apache.polaris.metrics.spi?
I believe the "owner" module ("metrics" in this case) has higher weight than "spi" because an SPI is needed by a particular module. I do not think we can talk about "general SPI" in Polaris. It is always grounded in a particular need in a particular module to access the pluggable service.
|
|
||
| polaris-bom=bom | ||
| polaris-core=polaris-core | ||
| polaris-spi=spi |
Contributor
There was a problem hiding this comment.
Similar to my other comment: polaris-spi feels too generic.
In this case we're adding a module needed by runtime/service, which is why I proposed to name it polaris-service-spi (path: runtime/service-spi) in the old comment thread. "Service" here meaning REST Catalog services.
Alternative: polaris-rest-spi (path: spi/rest).
Alternative: polaris-catalog-spi (path: spi/catalog).
WDYT?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
IcebergMetricsReporter is meant to be a plugin point: implement it your own way (log it, persist it, ship it to Kafka, whatever), and Polaris picks whichever one you've configured. PR #5068 built exactly that shape, but put the contract itself inside runtime/service, the Quarkus REST module, even though the interface has zero CDI, Quarkus, or JAX-RS in it, only guava, jspecify, and Iceberg's own types. I raised this in review (github.com/apache/polaris/pull/5068#discussion_r3676236578), and this PR moves the contract to polaris-core, alongside every other extension point, and moves the default logging implementation out to extensions/metrics-reports/base, next to its peers NoOpMetricsReporter and PersistingMetricsReporter. Writing a new reporter, or picking one of the three shipped ones, no longer touches runtime/service at all.
• Out of the box, Polaris used to log every scan and commit report it received, whether you asked for it or not. Defaulting to the loud reporter was never really a decision, it was just whichever one happened to already live in runtime/service. I flipped the default to the no-op reporter (the config value itself is still "default", it now just resolves to a different reporter), so a fresh install stays quiet unless you explicitly opt into logging (polaris.iceberg-metrics.reporting.type=logging) or persistence (=persisting).
• Selecting the logging reporter used to take two steps: pick it via config, then separately bump a log category to actually see anything, because that category defaulted to OFF. That override existed to keep brand-new installs quiet back when logging was the automatic default; once picking it is a deliberate choice, the override doesn't protect anyone. I dropped it, so selecting "logging" now logs right away, the same as selecting "persisting" persists right away. The override had also drifted to point at the wrong package, a bug I fixed separately: it targeted org.apache.polaris.service.reporting, a bare config interface with no logger, instead of LoggingMetricsReporter's real logger category, which its own javadoc already named.
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)