feat(persistence): Add JDBC persistence layer for Iceberg metrics reporting (#3337)#3385
feat(persistence): Add JDBC persistence layer for Iceberg metrics reporting (#3337)#3385obelix74 wants to merge 68 commits intoapache:mainfrom
Conversation
polaris-core/src/main/java/org/apache/polaris/core/storage/aws/AwsSessionTagsBuilder.java
Outdated
Show resolved
Hide resolved
persistence/relational-jdbc/src/main/resources/postgres/schema-v4.sql
Outdated
Show resolved
Hide resolved
persistence/relational-jdbc/src/main/resources/postgres/schema-v4.sql
Outdated
Show resolved
Hide resolved
runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java
Show resolved
Hide resolved
...me/service/src/main/java/org/apache/polaris/service/reporting/PersistingMetricsReporter.java
Outdated
Show resolved
Hide resolved
.../main/java/org/apache/polaris/persistence/relational/jdbc/models/MetricsReportConverter.java
Show resolved
Hide resolved
...me/service/src/main/java/org/apache/polaris/service/reporting/PersistingMetricsReporter.java
Outdated
Show resolved
Hide resolved
.../service/src/main/java/org/apache/polaris/service/reporting/MetricsReportCleanupService.java
Outdated
Show resolved
Hide resolved
|
Retest this please. |
461cfde to
2fc2a20
Compare
830085b to
fcf2716
Compare
|
@adutra, @dimas-b asked me to tag you to review the event related changes in this PR. Please see the google doc in the PR description for the background. Event-Related Changes Summary for Review This pull request introduces comprehensive metrics reporting events and infrastructure to enable audit logging of compute engine metrics (scan and commit reports from Spark, Trino, Flink, etc.). Below is a detailed breakdown of all event-related modifications: 1. Event Types (PolarisEventType.java) 2. Event Classes (IcebergRestCatalogEvents.java) 3. Event Listener Interface (PolarisEventListener.java) 4. Persistence Event Listener (PolarisPersistenceEventListener.java) 5. Event Service Delegator (IcebergRestCatalogEventServiceDelegator.java) 6. Test Event Listener (TestPolarisEventListener.java) 7. New Unit Tests (PolarisPersistenceEventListenerTest.java)
9. New Metrics Reporting Infrastructure The following new classes were added to support alternative metrics persistence strategies: File: runtime/service/src/main/java/org/apache/polaris/service/reporting/PolarisMetricsReporter.java File: runtime/service/src/main/java/org/apache/polaris/service/reporting/EventsMetricsReporter.java File: runtime/service/src/main/java/org/apache/polaris/service/reporting/PersistingMetricsReporter.java File: runtime/service/src/main/java/org/apache/polaris/service/reporting/CompositeMetricsReporter.java File: runtime/service/src/main/java/org/apache/polaris/service/reporting/MetricsReportCleanupService.java File: runtime/service/src/main/java/org/apache/polaris/service/reporting/MetricsReportingConfiguration.java Key Summary |
|
@dimas-b thanks for the help in merging #3414. I have rebased this PR against I think this PR can be split into two. PR 1: Enable metrics event emission PR 2: Add metrics persistence (depends on PR 1) If this helps, please let me know and I can split this PR into two. |
dimas-b
left a comment
There was a problem hiding this comment.
Preliminary comments, not an end-to-end review :)
...bc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java
Outdated
Show resolved
Hide resolved
...me/service/src/main/java/org/apache/polaris/service/reporting/PersistingMetricsReporter.java
Outdated
Show resolved
Hide resolved
runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java
Outdated
Show resolved
Hide resolved
|
@obelix74 : Re: splitting into two PRs - if it's not too much overhead, it might be preferable from my POV. I think persistence impl. may be a bit more involved as I commented above. |
On it. Moving this PR to draft until the event PR is merged and this branch / PR is rebased against main. |
|
35bbf1e to
1954427
Compare
@dimas-b I have further redone this PR into the following distinct commits. The feat/add-metrics-persistence-handler branch now contains 7 well-organized commits:
PR#1 is #3468. If need be, we can separate out each one of these self-contained commits into their own PRs for review. |
Address reviewer feedback from PR apache#3385 (comment r2700474938): - Introduce MetricsPersistence interface in polaris-core for backend-agnostic metrics persistence - Add MetricsContext immutable class to encapsulate metrics context information - Add NoOpMetricsPersistence for backends that don't support metrics persistence - Add JdbcMetricsPersistence implementation that wraps JdbcBasePersistenceImpl - Add MetricsPersistenceProducer CDI producer to select implementation at runtime - Refactor PersistenceMetricsProcessor to use MetricsPersistence instead of instanceof checks This design supports the upcoming NoSQL persistence backend (apache#3396) by allowing each backend to provide its own MetricsPersistence implementation or use the no-op implementation that silently discards metrics.
|
This PR for metrics persistence is now ready for review. PR1 (this PR): Database schema v4 and persistence layer for metrics storage. PR2 (follow-up, not raised yet): Metrics processing framework that uses this persistence layer. |
persistence/relational-jdbc/src/main/resources/postgres/schema-v4.sql
Outdated
Show resolved
Hide resolved
Replace junction tables (scan_metrics_report_roles, commit_metrics_report_roles) with a denormalized principal_role_ids JSON array column in both scan_metrics_report and commit_metrics_report tables. This simplifies the schema and reduces the number of tables from 5 to 3.
Remove BEFORE_REPORT_METRICS and AFTER_REPORT_METRICS event types as requested in PR review - events for metrics can be added in a separate PR if needed.
Per PR review feedback, MetricsPersistence is now injected directly via CDI instead of being obtained through MetaStoreManagerFactory.getOrCreateMetricsPersistence(). Changes: - Remove getOrCreateMetricsPersistence() from MetaStoreManagerFactory interface - Add @RequestScoped CDI producer for MetricsPersistence in ServiceProducers - Update PersistingMetricsReporter to inject MetricsPersistence directly - Update PersistingMetricsReporterTest to use direct injection This allows MetricsPersistence to be a request-scoped bean created on demand, independent of the entity persistence backend. Persistence backends that support metrics storage can provide an alternative CDI producer.
- Updated ModelScanMetricsReportTest and ModelCommitMetricsReportTest - Added tests for principal_role_ids JSON parsing in fromResultSet - Added tests for principal_role_ids serialization in toMap (H2 and Postgres) - Added tests for the separate Converter classes used in query methods - Updated createTestReport() to include roles
Sounds good. |
sounds ok while not enabled by default for me, longer term - and personally - I'd prefer an even (on kafka or alike) with a default small consumer storing it in a custom backend to decouple it and potentially scale independently 100% for audit concerns, but not a blocker at all |
Yes, completely different schema. Turned off by default. You have to explicitly pass a flag to create these tables. |
- Added unit tests for JdbcBootstrapUtils.shouldIncludeMetrics() method: - Test when schemaOptions is null (returns false) - Test when includeMetrics is true (returns true) - Test when includeMetrics is false (returns false) - Added integration tests in RelationalJdbcBootstrapCommandTest: - testBootstrapWithIncludeMetrics: verifies --include-metrics flag works - testBootstrapWithoutIncludeMetrics: verifies default behavior Per PR review comment from singhpk234
The schema-v4 version number fixes have been extracted to a separate branch (fix-schema-v4-version-number) for an independent PR. This reverts the version changes back to match main branch.
|
Just to sync up: The idea is to merge #3523 first, then this PR, right? |
Per PR review feedback from dimas-b: Polaris supports external IdP and PDP (e.g. Keycloak and OPA), and the roles stored in metrics tables may not be aligned with AuthZ decisions. Removed principal_role_ids column from both scan_metrics_report and commit_metrics_report tables in H2 and PostgreSQL schemas.
| * | ||
| * @param report the scan metrics report to persist | ||
| */ | ||
| public void writeScanMetricsReport(@Nonnull ModelScanMetricsReport report) { |
There was a problem hiding this comment.
Why not keep these methods in JdbcMetricsPersistence? 🤔
There was a problem hiding this comment.
Oh man, good idea. Did that in a commit. Thanks again.
runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java
Outdated
Show resolved
Hide resolved
Per PR review feedback from dimas-b: Polaris supports external IdP and PDP (e.g. Keycloak and OPA), and the roles stored in metrics tables may not be aligned with AuthZ decisions. Removed: - PRINCIPAL_ROLE_IDS column constant from model classes - getRoles() method from model interfaces - Roles parsing/serialization from fromResultSet() and toMap() - Roles-related tests from test classes
Addresses PR review comments from dimas-b: - r2775335558: Why not keep these methods in JdbcMetricsPersistence? - r2775346205: Do you plan wiring JdbcMetricsPersistence to CDI? Changes: - Move all metrics persistence methods from JdbcBasePersistenceImpl to JdbcMetricsPersistence, making it self-contained - Create JdbcMetricsPersistenceProducer as a CDI producer that creates JdbcMetricsPersistence instances for the relational-jdbc backend - Update ServiceProducers.metricsPersistence() to use Instance<MetricsPersistence> pattern to select the appropriate implementation based on persistence type - Update MetricsReportPersistenceTest to use JdbcMetricsPersistence directly
Address PR review comment - remove namespace since tableId uniquely identifies the table. Namespace can be derived from the table entity if needed. - Remove namespace column from H2 and Postgres schema-metrics-v1.sql - Remove namespace from Model classes (ModelScanMetricsReport, ModelCommitMetricsReport) - Remove namespace from converter classes - Update SpiModelConverter to not use namespace - Update PersistingMetricsReporter to not pass namespace - Update all related tests
dimas-b
left a comment
There was a problem hiding this comment.
Some more CDI comments. I'll make another review pass after the prerequisite PRs are merged and this one is rebased.... LGTM in general 👍
| if (selected.isResolvable()) { | ||
| return selected.get(); | ||
| } | ||
| return MetricsPersistence.NOOP; |
There was a problem hiding this comment.
I'd rather error out if the configured instance is not resolvable. We do that pretty much in all other cases.
NOOP could be selected with a default value in application.properties.
| public MetricsPersistence metricsPersistence( | ||
| PersistenceConfiguration config, @Any Instance<MetricsPersistence> metricsPersistenceImpls) { | ||
| Instance<MetricsPersistence> selected = | ||
| metricsPersistenceImpls.select(Identifier.Literal.of(config.type())); |
There was a problem hiding this comment.
Would it make sense to use a different config for MetricsPersistence? We came some way to detach it from the JDBC metastore, I believe it would be nice to also make it selectable independently.
There was a problem hiding this comment.
Created MetricsPersistenceConfiguration for this.
| .openInitScriptResource(effectiveSchemaVersion)); | ||
|
|
||
| // Run the metrics schema script if requested | ||
| if (JdbcBootstrapUtils.shouldIncludeMetrics(bootstrapOptions)) { |
There was a problem hiding this comment.
Just to sync up and not forget: I believe it would be nice to have a separate bootstrap handler for the metrics schema, but that can be done as a follow-up PR.
There was a problem hiding this comment.
Implemented in commit c23b162e5. Created a MetricsSchemaBootstrap SPI interface in polaris-core with bootstrap(realmId) and isBootstrapped(realmId) methods, plus a NOOP constant. The JDBC implementation (JdbcMetricsSchemaBootstrap) is idempotent and uses the metrics_version table to track bootstrap state.
Also added a new bootstrap-metrics CLI command that allows operators to bootstrap the metrics schema independently:
polaris-admin bootstrap-metrics -r my-realm
This enables adding metrics persistence support to existing deployments without re-bootstrapping the entity schema.
runtime/admin/src/main/java/org/apache/polaris/admintool/config/AdminToolProducers.java
Outdated
Show resolved
Hide resolved
- Create separate MetricsPersistenceConfiguration with polaris.persistence.metrics prefix and 'noop' default value (addresses r2775695727) - Add NoopMetricsPersistence CDI bean with @Identifier("noop") annotation - Update ServiceProducers.metricsPersistence() to error if type not resolvable instead of falling back to NOOP (addresses r2775688321) - Refactor AdminToolProducers: move UUID to dummyRealmContext() and have dummyRealmConfig() take RealmContext as parameter (addresses r2775725538)
| * @see MetricsPersistence#NOOP | ||
| */ | ||
| @ApplicationScoped | ||
| @Identifier("noop") |
There was a problem hiding this comment.
optional: We can probably have a simple producer method in ServiceProducers having this annotation and returning MetricsPersistence.NOOP - seems simpler than a delegator class.
There was a problem hiding this comment.
Done. Replaced the NoopMetricsPersistence delegator class with a simple producer method in ServiceProducers.
This commit implements a decoupled bootstrap handler for the metrics schema, following dimas-b's suggestion in PR review comment r2775702215. Changes: - Create MetricsSchemaBootstrap SPI interface in polaris-core - Defines bootstrap(realmId) and isBootstrapped(realmId) methods - Includes NOOP constant for backends that don't support metrics - Annotated with @beta since the API is experimental - Create JdbcMetricsSchemaBootstrap implementation in polaris-relational-jdbc - Executes schema-metrics-v1.sql for H2/PostgreSQL - Idempotent: checks metrics_version table before bootstrapping - Uses DatasourceOperations for database access - Create MetricsSchemaVersion model class for metrics_version table - Add generateMetricsVersionQuery() to QueryGenerator - Add CDI producers for MetricsSchemaBootstrap - JdbcMetricsPersistenceProducer: produces @Identifier("relational-jdbc") - ServiceProducers: produces @Identifier("noop") - Update JdbcMetaStoreManagerFactory to use injected MetricsSchemaBootstrap instead of inline metrics bootstrap logic - Add 'bootstrap-metrics' CLI command (BootstrapMetricsCommand) - Allows operators to bootstrap metrics schema independently - Supports multiple realms: -r realm1 -r realm2 - Idempotent: skips already-bootstrapped realms - Simplify NoopMetricsPersistence (per r2776432583) - Replace delegator class with simple producer method in ServiceProducers - Delete NoopMetricsPersistence.java This enables operators to add metrics persistence support to existing Polaris deployments without re-bootstrapping the entity schema.
| // RelationalJdbcBootstrapCommandTest#testBootstrapFailsWhenAddingRealmWithDifferentSchemaVersion | ||
| // | ||
| // @Test | ||
| // public void testBootstrapMetricsIdempotent(QuarkusMainLauncher launcher) { |
There was a problem hiding this comment.
I really can't test state persistence across launches because of this.
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)Description
This PR introduces the JDBC persistence layer for storing Iceberg metrics reports (scan and commit operations) as part of the Compute Client Audit Reporting feature (#3337).
Overview
This change adds the foundational persistence infrastructure required to store Iceberg metrics data received via the REST catalog's /reportMetrics endpoint. The metrics data includes detailed information about scan operations (file counts, data volumes, filter expressions) and commit operations (added/removed files, snapshot details).
Components Introduced
Model Classes
• ModelScanMetricsReport - Immutable model representing scan metrics with 30+ fields
• ModelCommitMetricsReport - Immutable model representing commit metrics with 30+ fields
Converters
• MetricsReportConverter - Converts Iceberg ScanReport and CommitReport objects to persistence models
• ModelScanMetricsReportConverter - Maps JDBC ResultSet to ModelScanMetricsReport
• ModelCommitMetricsReportConverter - Maps JDBC ResultSet to ModelCommitMetricsReport
Persistence Methods (added to JdbcBasePersistenceImpl)
• writeScanMetricsReport() / writeCommitMetricsReport() - Insert metrics into the database
• queryScanMetricsReports() / queryCommitMetricsReports() - Query metrics by realm and catalog
• queryScanMetricsReportsByTraceId() / queryCommitMetricsReportsByTraceId() - Query metrics by OpenTelemetry trace ID
• deleteScanMetricsReportsOlderThan() / deleteCommitMetricsReportsOlderThan() - Time-based cleanup
• deleteAllMetricsReportsOlderThan() - Unified cleanup for both metric types
Testing
Related Issues