feat(metrics): Evolve PolarisMetricsReporter interface with timestamp parameter and comprehensive documentation by obelix74 · Pull Request #3468 · apache/polaris

obelix74 · 2026-01-17T03:17:52Z

This PR enhances the PolarisMetricsReporter SPI interface by adding a timestamp parameter to the reportMetric() method, enabling accurate time-series metrics reporting to external systems.

Checklist

🛡️ Don't disclose security issues! (contact security@apache.org)
🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
🧪 Added/updated tests with good coverage, or manually tested (and explained how)
💡 Added comments for complex logic
🧾 Updated CHANGELOG.md (if needed)
📚 Updated documentation in site/content/in-dev/unreleased (if needed)

This commit adds the core infrastructure for emitting metrics as events when reportMetrics() is called on the Iceberg REST catalog API. Changes: - Add REPORT_METRICS_REQUEST attribute to EventAttributes.java - Add BEFORE_REPORT_METRICS and AFTER_REPORT_METRICS to PolarisEventType.java - Update reportMetrics() in IcebergRestCatalogEventServiceDelegator.java to emit BEFORE/AFTER events with catalog name, namespace, table, and request - Add ReportMetricsEventTest.java with unit tests verifying event emission This enables event listeners to receive metrics report events, allowing for use cases like audit logging and metrics persistence. Added tests and a Feature flag

obelix74 · 2026-01-17T03:21:11Z

@dimas-b Created this PR for the event emission and added a feature flag for backwards compatibility.

...java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java

adutra · 2026-01-19T09:19:42Z

@dimas-b Created this PR for the event emission and added a feature flag for backwards compatibility.

I am not sure backwards compatibility is important here as currently no listener bundled with Polaris actually processes the metrics-related events.

...java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java

@Identifier

This commit removes the event-based metrics reporting system and introduces a new MetricsProcessor interface with CDI support. This is the foundation for a simpler, more direct metrics processing architecture. Changes: - Remove ENABLE_METRICS_EVENT_EMISSION feature flag - Remove BEFORE_REPORT_METRICS and AFTER_REPORT_METRICS event types - Remove REPORT_METRICS_REQUEST event attribute - Remove event emission from IcebergRestCatalogEventServiceDelegator.reportMetrics() - Remove ReportMetricsEventTest - Add MetricsProcessor interface for processing metrics reports - Add MetricsProcessingContext with rich contextual information (realm ID, principal, request ID, OpenTelemetry trace context) - Add MetricsProcessorConfiguration for type-safe configuration - Add CDI producer in ServiceProducers for MetricsProcessor The new MetricsProcessor interface provides: - Simpler, more direct processing (no events) - Rich context with realm, principal, request ID, OTel trace - CDI-based extensibility via @Identifier annotations - Type-safe configuration Implementations will be added in subsequent PRs. This commit provides the foundational interfaces and CDI infrastructure. Backward compatibility: The existing PolarisMetricsReporter interface and configuration remain unchanged and functional.

- Remove ENABLE_METRICS_EVENT_EMISSION feature flag entry - Add polaris.metrics.processor.type configuration property

runtime/service/src/main/java/org/apache/polaris/service/reporting/MetricsProcessor.java

.polaris-work-notes.md

obelix74 · 2026-01-19T17:55:49Z

@adutra and @dimas-b thanks for all the feedback. Indeed, @singhpk234 also did not like the event based approach. I have made the following changes to address your feedback.

Introduces a new MetricsProcessor interface with CDI support. This establishes the foundation for a simpler, more direct metrics processing architecture. I have intentionally left out PolarisMetricsReporter.
MetricsProcessingContext - Rich context object providing:
• Catalog and table information
• Realm ID and principal name
• Request ID for correlation
• OpenTelemetry trace ID and span ID
• Timestamp
• MetricsProcessorConfiguration - Type-safe configuration using Quarkus @ConfigMapping
• CDI producer in ServiceProducers for MetricsProcessor selection via @Identifier annotations

The existing PolarisMetricsReporter interface and polaris.iceberg-metrics.reporting.type configuration remain unchanged and functional.

What's Not Included

This PR intentionally excludes implementations to keep the scope focused:
• No processor implementations (noop, logging, persistence)
• No integration with IcebergCatalogAdapter
• No backward compatibility adapters

These will be added in subsequent PRs.

Configuration

   polaris:
     metrics:
       processor:
         type: noop  # Default - implementations will be added in follow-up PRs

Next Steps

Follow-up PRs will add:

Built-in processor implementations (noop, logging, persistence)
Integration with the REST catalog adapter
Backward compatibility layer for existing PolarisMetricsReporter users

I am open to removing PolarisMetricsReporter - but need to figure out how to migrate existing code and users. In fact, I just upgraded my Polaris installation to 1.3.0 to use the metrics logging output while we sort out these.

obelix74 · 2026-01-19T18:06:54Z

Perhaps I should keep the polaris.iceberg-metrics.reporting.type property and not introduce a new property. That way we can delete any existing code and not break anyone else. I think I was trying to be too conservative in introducing a new property, but that adds more work. @adutra what do you think?

...ime/service/src/main/java/org/apache/polaris/service/reporting/MetricsProcessingContext.java

runtime/service/src/main/java/org/apache/polaris/service/reporting/MetricsProcessor.java

runtime/service/src/main/java/org/apache/polaris/service/reporting/PolarisMetricsReporter.java

.../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java

runtime/service/src/main/java/org/apache/polaris/service/reporting/PolarisMetricsReporter.java

...java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java

CHANGELOG.md

runtime/service/src/main/java/org/apache/polaris/service/reporting/PolarisMetricsReporter.java

.../testFixtures/java/org/apache/polaris/service/events/listeners/TestPolarisEventListener.java

dimas-b

LGTM with a couple of remaining minor comments. Thanks for bearing with me, @obelix74 !

@adutra : WDYT?

adutra

LGTM as well, thank you @obelix74 !

dimas-b

LGTM 👍 Thanks again, @obelix74 !

Let's give this PR another day in review in case other people have comments.

CC: @cccs-cat001

singhpk234

Thank you @obelix74 for the change it mostly LGTM too !

singhpk234 · 2026-01-21T16:38:10Z

runtime/service/src/main/java/org/apache/polaris/service/reporting/PolarisMetricsReporter.java

+ * @see MetricsReportingConfiguration
+ */
 public interface PolarisMetricsReporter {
-  public void reportMetric(String catalogName, TableIdentifier table, MetricsReport metricsReport);


we release this public interface in 1.3 right ? i wonder if we keep this api with default impl to the new method with null or something ?
361b7e9

@singhpk234 I had a deprecated backward compatible method and the consensus was to remove it since this SPI is not meant to be used by developers outside Polaris.

Didn't fully get it, wdym by developers outside Polaris ? can this not be used downstream projects, my recommendation is to make sure version upgrades are seemless so keep this and add more if we want and just update the default of this work under timestamp null assumption.

Let me also check previous discussions, can you please point me to that ?

@singhpk234 #3468 (comment). @dimas-b FYI.

Per our standing evolution guidelines public classes / methods can change in any release. Unlike REST API changes, it is not considered a "major" change for versioning purposes. Essentially java methods are not part of the API surface in the SemVer sense.

We should and do try to make java API changes in a backward -compatible manner when practical.

Regarding this particular PR and java interfaces that are part of an SPI (defined and called by Polaris, implemented by 3rd party plugins), I do not see a practical way to evolve them in a backward-compatible manner without causing excessive maintenance burden in Polaris code.

In this case, Polaris would have to define a new interface for the new method signature and perform instanceof checks at runtime in order to decide whether to call the old or the new method. I do believe it would be an overkill at the current stage of the project (still evolving actively). It is not too hard for downstream implementations to adjust code for the newly added parameter.

That said, I'd like to propose moving the SPI evolution discussion to the dev ML as it is a big and complex topic.

@singhpk234 : please clarify whether you consider this comment thread a blocker for merging (or not).

@dimas-b can we not add the api in the same interface and add the default impl of old api call this null clock, i would like to understand this more if this is possible. If its not possible i am supportive of the change !

@singhpk234 : please clarify whether you consider this comment thread a blocker for merging (or not).

thanks for asking the clarification i believe anything is thats not marked as 'nit' / 'not blocker' / 'not' is expected to be resolved before merging (per here) , i really appreciate you checking in !

can we not add the api in the same interface and add the default impl of old api call this null clock [...]

I do not see a point in adding a default impl for the old method alone. Existing implementations will have overrides for it already (javac will not let them miss that).

We could add a default impl. for the new method and redirect to the old one (without the Instant). This will allow existing implementations to compile without changes. However, this creates uncertainty for the implementor regarding which method should do the real work. Javadoc helps, but adds cognitive load. We could add default to both methods and deprecate the old one, but this will add "cruft" to the interface definition, when, I believe, adapting to the new interface in downstream projects is very easy.

Please note that providing a custom implementation for this interface requires a downstream build. So all upgrades will go through local builds and CI (assuming normal software engineering practices), where the need for adjustments will be become apparent.

When I uptook 1.3.0, I was glad that my compilation failed since if the old method was deprecated, I would be confused why I need to uptake the new approach and how to do it.

Having said that, this PR started as adding events for metrics, we removed it. It only contains this extra attribute. It is not a blocker for me. If we don't agree on how to proceed, I would rather abandon this PR and focus on the simplified metrics persistence code here: #3385. That allows Polaris to persist table metrics to the database.

Thats fair, though in this case we are forcing the upgrades to have this new interface implementation when having the old api was not hurting ? taking the case LOGGERs can be configured to implicitly log timestamp additionally and the way we have reporting wired if i wanna do custom one can just do timestamp internally rather than requesting from the signature ?

metricsReporter.reportMetric( catalogName, tableIdentifier, reportMetricsRequest.report(), clock.instant());

it not the time when the request hit the server but its the time when the reportMetric is called so does it matter of i do this vs

reportMetric(catalogName, tableIdentifier, reportMetricsRequest.report())) {
timestamp = clock.instant()
}

with that being said i think its fine if we wanna move forward :), but above is my thought process. Please move forward @dimas-b trust your judgement here !

singhpk234 · 2026-01-21T16:39:36Z

CHANGELOG.md

 - The EclipseLink Persistence implementation has been completely removed.
 - The default request ID header name has changed from `Polaris-Request-Id` to `X-Request-ID`.
 - The (Before/After)CommitTableEvent has been removed.
+- The `PolarisMetricsReporter.reportMetric()` method signature has been extended to include a


i belive we need to freeze the changelog for 1.3 since its released and then add this in an unrelease section ?

I will move it to unreleased section.

@singhpk234 I don't see a 1.3.0 section in the CHANGELOG - I synced my fork to upstream.

https://github.com/apache/polaris/blob/main/CHANGELOG.md

Am I missing something?

Perhaps we need to rename Unreleased to 1.3.0-incubating in CHANGELOG and move this line alone to Unreleased.

no you are not ideally there needs to a update in the main branch to freeze 1.3 section and create the unreleased section as soon as a version of Polaris is released, seems like we need to do it first ?

As far as the CHANGELOG "diff" in this PR is concerned, it looks correct to me.

I'd expect @pingtimeout (as the release manager for 1.3.0) to open a reconciliation PR for main and add a dedicated section for 1.3.0 soon 😉 I'm sure @pingtimeout will be able to deal with conflicts if this PR merges first 🙂

@obelix74 : please do not add a 1.3.0 section to CHANGELOG in this PR :)

@dimas-b Noted.

cccs-cat001

Great idea! I appreciate the improvement :)

dimas-b · 2026-01-22T18:36:16Z

It looks like we have consensus in all discussions and some approvals. If no new concerns are raised. I propose to merge on Jan 23.

obelix74 · 2026-01-22T18:45:26Z

Thanks all.

dimas-b · 2026-01-23T15:31:16Z

@obelix74 : unfortunately, it looks like there's a conflict on CHANGELOG.md now... Could you resolve it?

obelix74 · 2026-01-23T15:49:11Z

@obelix74 : unfortunately, it looks like there's a conflict on CHANGELOG.md now... Could you resolve it?

Resolved.

… parameter and comprehensive documentation (apache#3468) Enhance the `PolarisMetricsReporter` SPI interface by adding a timestamp parameter to the `reportMetric()` method, enabling accurate time-series metrics reporting to external systems.

github-project-automation bot added this to Basic Kanban Board Jan 17, 2026

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Jan 17, 2026

obelix74 mentioned this pull request Jan 17, 2026

feat(persistence): Add JDBC persistence layer for Iceberg metrics reporting (#3337) #3385

Open

6 tasks

adutra reviewed Jan 19, 2026

View reviewed changes

...java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java Outdated Show resolved Hide resolved

dimas-b reviewed Jan 19, 2026

View reviewed changes

...java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java Outdated Show resolved Hide resolved

Anand Kumar Sankaran added 3 commits January 19, 2026 09:31

Removed internal document

aa43beb

docs: Update CHANGELOG for metrics processor configuration

489a140

- Remove ENABLE_METRICS_EVENT_EMISSION feature flag entry - Add polaris.metrics.processor.type configuration property

adutra reviewed Jan 19, 2026

View reviewed changes

runtime/service/src/main/java/org/apache/polaris/service/reporting/MetricsProcessor.java Outdated Show resolved Hide resolved

adutra reviewed Jan 19, 2026

View reviewed changes

.polaris-work-notes.md Outdated Show resolved Hide resolved

obelix74 changed the title ~~feat(events): Enable metrics event emission in reportMetrics()~~ feat(events): New metrics reporter interface with multiple strategies Jan 19, 2026

obelix74 requested review from adutra and dimas-b January 19, 2026 17:56

obelix74 changed the title ~~feat(events): New metrics reporter interface with multiple strategies~~ feat(metrics): New metrics reporter interface with multiple strategies Jan 19, 2026

SpotlessApply

ea47412

dimas-b reviewed Jan 19, 2026

View reviewed changes

Review comments

772e6fb

obelix74 requested a review from dimas-b January 19, 2026 23:34

dimas-b reviewed Jan 19, 2026

View reviewed changes

runtime/service/src/main/java/org/apache/polaris/service/reporting/PolarisMetricsReporter.java Outdated Show resolved Hide resolved

Review comments

f3bd22e

obelix74 commented Jan 19, 2026

View reviewed changes

runtime/service/src/main/java/org/apache/polaris/service/reporting/PolarisMetricsReporter.java Show resolved Hide resolved

obelix74 requested a review from dimas-b January 20, 2026 00:00

obelix74 changed the title ~~feat(metrics): New metrics reporter interface with multiple strategies~~ feat(metrics): Evolve PolarisMetricsReporter interface with timestamp parameter and comprehensive documentation Jan 20, 2026

Added a service producer for SpanContext

194150c

obelix74 commented Jan 20, 2026

View reviewed changes

runtime/service/src/main/java/org/apache/polaris/service/reporting/PolarisMetricsReporter.java Outdated Show resolved Hide resolved

adutra reviewed Jan 20, 2026

View reviewed changes

.../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogAdapter.java Outdated Show resolved Hide resolved

Review comments about using Clock

d69c08a

obelix74 requested a review from adutra January 20, 2026 18:58

dimas-b reviewed Jan 20, 2026

View reviewed changes

Review comments.

f2e2379

dimas-b reviewed Jan 20, 2026

View reviewed changes

runtime/service/src/main/java/org/apache/polaris/service/reporting/PolarisMetricsReporter.java Outdated Show resolved Hide resolved

dimas-b reviewed Jan 20, 2026

View reviewed changes

.../testFixtures/java/org/apache/polaris/service/events/listeners/TestPolarisEventListener.java Outdated Show resolved Hide resolved

dimas-b reviewed Jan 20, 2026

View reviewed changes

Review comments.

64b52e0

obelix74 requested a review from dimas-b January 20, 2026 20:48

Anand Kumar Sankaran added 2 commits January 20, 2026 13:10

spotlessApply: Remove trailing blank line

cbeb620

Merge branch 'main' into feat/enable-metrics-event-emission

1a3c2b2

adutra approved these changes Jan 21, 2026

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Jan 21, 2026

dimas-b previously approved these changes Jan 21, 2026

View reviewed changes

singhpk234 reviewed Jan 21, 2026

View reviewed changes

Merge branch 'main' into feat/enable-metrics-event-emission

a7c8363

cccs-cat001 approved these changes Jan 22, 2026

View reviewed changes

adutra previously approved these changes Jan 22, 2026

View reviewed changes

Merge branch 'main' into feat/enable-metrics-event-emission

bd15dcd

obelix74 dismissed stale reviews from adutra and dimas-b via bd15dcd January 23, 2026 15:48

dimas-b approved these changes Jan 23, 2026

View reviewed changes

dimas-b merged commit d36e88e into apache:main Jan 23, 2026
15 checks passed

github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Jan 23, 2026

Conversation

obelix74 commented Jan 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

obelix74 commented Jan 17, 2026

Uh oh!

Uh oh!

adutra commented Jan 19, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

obelix74 commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

obelix74 commented Jan 19, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

adutra left a comment

Choose a reason for hiding this comment

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

singhpk234 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

obelix74 commented Jan 17, 2026 •

edited

Loading

obelix74 commented Jan 19, 2026 •

edited

Loading

dimas-b Jan 21, 2026 •

edited

Loading