(feat) Implement metrics rest api

[obelix74]

What / Why

Part of the 3-PR split of the Iceberg metrics work (agreed with EJ, Dmitri, Anand):

• PR0 — refactor(metrics): relocate Iceberg metrics write path to extensions with enveloped core SPI (PR0 of 3) #5068 — reporter SPI refactor + write-path relocation.
• PR1 (this PR) — Table Metrics query REST API.
• PR2 — feat(metrics): durable JDBC metrics extension (Scope 2) #4756 — JDBC implementation of the query SPI.

Depends on #5068. This branch is stacked on the PR0 branch, so until #5068 merges the diff here also shows PR0's changes. Review the commits from feat(metrics): add Table Metrics Reports query REST API (PR1) onward.

Changes

• OpenAPI spec + generated module under extensions/metrics-reports/api (polaris-extensions-metrics-reports-api) — the metrics API ships as an optional, extension-scoped feature.
• MetricsQuerySpi in extensions/metrics-reports/spi; a no-op default (NoOpMetricsQuery, @DefaultBean) in extensions/metrics-reports/base so the read path returns an empty page (HTTP 200) until a durable backend is installed.
• Thin HTTP→SPI handler MetricsReportsService in runtime/service (resolves names→ids, authorizes, delegates to MetricsQuerySpi).
• Read-path authorization in polaris-core: TABLE_READ_METRICS privilege + LIST_TABLE_METRICS operation, wired through the authorizer/RBAC and mirrored in the Ranger extension.

Testing

• ./gradlew build and :polaris-runtime-service:intTest (Quarkus) pass.

[dimas-b]

Thanks for pushing this feature forward, @obelix74 !

The PR LGTM in general. Posting some comments about code organization, subject to discussion, of course.

[dimas-b]

LGTM with one minor remaining comment 😅

@sungwy , @sneethiraj : FYI about the new AuthZ operation.

[flyingImer]

The direction looks right to me

Two structural observations:

• With this PR, MetricsPersistence grows from 2 write methods to 4 (read + write). It's marked @beta and the javadoc calls it a "Service Provider Interface." But it lives on BasePersistence, which only local DB backends implement. NoSqlMetaStoreManager and RemotePolarisMetaStoreManager go through empty BasePersistence implementations, so these methods are permanently no-op for them. Meanwhile, the actual SPI interfaces (PolarisMetricsReporter, PolarisMetricsManager) have no annotation at all. The @beta signal is on the wrong layer IIUC

• The write path enters through PolarisMetricsManager on MetaStoreManager, but this read path bypasses that layer and goes straight to BasePersistence via callContext.getMetaStore(). If we want the metrics read API to work for non-JDBC backends, it would need a MetaStoreManager-level entry point, same as writes.

Not blocking on this. I think the question of where metrics persistence should sit architecturally is worth a discussion on dev@.

[obelix74]

The direction looks right to me

Two structural observations:

• With this PR, MetricsPersistence grows from 2 write methods to 4 (read + write). It's marked @beta and the javadoc calls it a "Service Provider Interface." But it lives on BasePersistence, which only local DB backends implement. NoSqlMetaStoreManager and RemotePolarisMetaStoreManager go through empty BasePersistence implementations, so these methods are permanently no-op for them. Meanwhile, the actual SPI interfaces (PolarisMetricsReporter, PolarisMetricsManager) have no annotation at all. The @beta signal is on the wrong layer IIUC
• The write path enters through PolarisMetricsManager on MetaStoreManager, but this read path bypasses that layer and goes straight to BasePersistence via callContext.getMetaStore(). If we want the metrics read API to work for non-JDBC backends, it would need a MetaStoreManager-level entry point, same as writes.

Not blocking on this. I think the question of where metrics persistence should sit architecturally is worth a discussion on dev@.

Thank you. I have added @Beta annotation to PolarisMetricsManager and PolarisMetricsReporter.

About the second point, thank you. Would this mean a read method to PolarisMetricsManager and MetaStoreManager mirroring the write path? Should I do it in this PR or can this wait?

[flyingImer]

The direction looks right to me
Two structural observations:

• With this PR, MetricsPersistence grows from 2 write methods to 4 (read + write). It's marked @beta and the javadoc calls it a "Service Provider Interface." But it lives on BasePersistence, which only local DB backends implement. NoSqlMetaStoreManager and RemotePolarisMetaStoreManager go through empty BasePersistence implementations, so these methods are permanently no-op for them. Meanwhile, the actual SPI interfaces (PolarisMetricsReporter, PolarisMetricsManager) have no annotation at all. The @beta signal is on the wrong layer IIUC
• The write path enters through PolarisMetricsManager on MetaStoreManager, but this read path bypasses that layer and goes straight to BasePersistence via callContext.getMetaStore(). If we want the metrics read API to work for non-JDBC backends, it would need a MetaStoreManager-level entry point, same as writes.

Not blocking on this. I think the question of where metrics persistence should sit architecturally is worth a discussion on dev@.

Thank you. I have added @Beta annotation to PolarisMetricsManager and PolarisMetricsReporter.

About the second point, thank you. Would this mean a read method to PolarisMetricsManager and MetaStoreManager mirroring the write path? Should I do it in this PR or can this wait?

Thanks for adding @beta.

Reads should go through MetaStoreManager too, same as writes. If reads stay on BasePersistence, non-JDBC backends can't implement the read API at all. I'd prefer fixing that in this PR so the read path ships with the same layering as writes.

Separately, the persistence schema discussion on dev@ is still open. A follow-up issue linking to that thread would help track it.

[obelix74]

Reads should go through MetaStoreManager too, same as writes. If reads stay on BasePersistence, non-JDBC backends can't implement the read API at all. I'd prefer fixing that in this PR so the read path ships with the same layering as writes.

Separately, the persistence schema discussion on dev@ is still open. A follow-up issue linking to that thread would help track it.

Pushed a commit (and rebased against updated main). listScanMetrics and listCommitMetrics are now on PolarisMetricsManager (and therefore MetaStoreManager), following the same pattern as the write methods. MetricsReportsService now injects PolarisMetaStoreManager and routes reads through it rather than calling callContext.getMetaStore() directly.

For the persistence schema discussion — I'll open a follow-up issue linking to the dev@ thread once there's a message to reference. Happy to do that now if you can share the thread link (I don't have it handy). Please let me know.

[dimas-b]

The metrics API story LGTM 👍

I still have more general concerns related to SPI design and service wiring, but they are not specific to this feature.

Thanks for working on this @obelix74 !

[flyingImer]

Are you planning to merge as-is now that Dmitri approved, or is there another round? Asking because the May 7 metrics sync landed on a few directional items that touch the schema and SPI shape here. Left some questions inline.

[dimas-b]

@obelix74 : it looks like this PR got a lot of conflicts 🤷

[obelix74]

@obelix74 : it looks like this PR got a lot of conflicts 🤷

Resolved all conflicts and push. Rebased against main.

[flyingImer]

Thanks for continuing to push this forward. The direction still looks good to me: exposing persisted metrics through a beta read API, using a stable response envelope, keeping the API in an extension module, and routing reads through table-scoped authz all make sense.

One thing I would still like to clarify before this merges is sequencing with the metrics SPI/schema work we discussed after the May 7 sync.

From the previous review thread, I think we already converged on a few points:

• the current MetricsPersistence / PolarisMetricsManager layering is transitional, and #4397 is expected to move metrics persistence out of the old aggregated BasePersistence shape;
• the current scan_metrics_report / commit_metrics_report split is also transitional, with the follow-up direction being a single metrics report model/table with a metric type discriminator;
• listScanMetrics / listCommitMetrics are therefore likely interim API/SPI shapes, and may collapse or be rerouted when the schema/SPI consolidation happens.

I don't think this PR has to solve all of that before it can make progress. But I do think we should avoid accidentally standardizing the transitional shape just because this PR is ready first.

Could we make the sequencing explicit before merge? For example, either rebase on #4397 if that lands first, or link a concrete follow-up that tracks:

1. moving metrics persistence to the standalone SPI shape,
2. consolidating the metrics schema/model,
3. deciding whether the per-type list methods remain public SPI surface or collapse behind a typed query API.

[dimas-b]

Closed by mistake

[flyingImer]

should scan and commit just being enveloped rather than individual SPI method?

could you please remind me the rationale of you choosing one over the other?

[obelix74]

Good question — the two-method shape here mirrors MetricsPersistence (the write-side SPI this interface is the read counterpart to), which also has two separate methods, writeScanReport/writeCommitReport, each taking its own concrete record type rather than an envelope.

The enveloping in IcebergMetricsReporter/MetricsReportEnvelope solves a different problem: it's the single ingestion entry point that receives whichever Iceberg report type shows up at runtime, so it needs one polymorphic signature with an explicit MetricType discriminator for callers to branch on. PersistingMetricsReporter (which implements IcebergMetricsReporter) unwraps that envelope and dispatches to the strongly-typed MetricsPersistence write methods.

MetricsQuerySpi doesn't have that "single arrival point" problem — callers already know whether they want scan or commit reports (it's the metricType query param), so there's no runtime type to discriminate. Keeping listScanReports/listCommitReports as separate, strongly-typed methods avoids introducing a union/envelope type on the read side purely to unwrap it again one line later, and keeps it symmetric with the write SPI it's paired with.

Happy to reconsider if you see a concrete benefit to enveloping here that I'm missing.

[obelix74]

Good catch — collapsed listScanReports/listCommitReports into a single enveloped listReports(MetricType, ...) returning Page<? extends MetricsRecordIdentity>, matching the discriminated envelope the REST layer already uses (ListMetricsResponse oneOf). See 4a0b224.

[flyingImer]

I agree with collapsing this to one envelope-based method. The remaining gap is that MetricType and the returned Page are still unrelated values, so a provider can pair COMMIT with scan records and the service has to recover the relationship through casts. Could listMetrics return a sealed QueryResult with typed ScanResult and CommitResult variants, with metricType derived from the variant? That keeps one entry point, makes an invalid envelope fail to compile, and lets the service use an exhaustive switch without casts. One check at the SPI seam can verify that the returned variant matches the requested runtime MetricType.

For example:

sealed interface QueryResult permits ScanResult, CommitResult {
  MetricType metricType();
}

record ScanResult(Page<ScanMetricsRecord> reports)
    implements QueryResult {

  @Override
  public MetricType metricType() {
    return MetricType.S.SCAN;
  }
}

record CommitResult(Page<CommitMetricsRecord> reports)
    implements QueryResult {

  @Override
  public MetricType metricType() {
    return MetricType.COMMIT;
  }
}

QueryResult listReports(
    MetricType metricType,
    long catalogId,
    List<Long> tableIds,
    Long snapshotId,
    Long timestampFrom,
    Long timestampTo,
    PageToken pageToken);

This way will safeguard such:

new CommitResult(scanPage); // compile error

service no longer needs to cast:

QueryResult result = provider.listReports(type, ...);

checkState(
    result.metricType() == type,
    "Provider returned %s for %s",
    result.metricType(),
    type);

return switch (result) {
  case ScanResult scan ->
      toScanResponse(scan.reports());

  case CommitResult commit ->
      toCommitResponse(commit.reports());
};

no op can simply be:

return switch (metricType) {
  case SCAN ->
      new ScanResult(Page.fromItems(List.of()));

  case COMMIT ->
      new CommitResult(Page.fromItems(List.of()));
};

[obelix74]

Implemented as suggested in 975c01c (refactor(metrics): make MetricsQuerySpi.listReports return a typed sealed QueryResult):

• listReports now returns a sealed QueryResult with ScanResult/CommitResult variants, each carrying a strongly-typed Page<ScanMetricsRecord>/Page<CommitMetricsRecord> and deriving metricType() from the variant.
• MetricsReportsService checks result.metricType() == type and then switches exhaustively over ScanResult/CommitResult — no casts.
• NoOpMetricsQuery returns new ScanResult(...)/new CommitResult(...) directly via a switch on the requested metricType, matching your proposed no-op shape.

Thanks for the detailed sketch — implemented essentially as written.

[obelix74]

It's intentional, not merge noise — this is the direct answer to your earlier ask on this thread: #4115 (comment) ("Could we provide an operator-consumable service-definition update and test LIST_TABLE_METRICS against that exact artifact?").

The problem it closes: authz_tests/dev_polaris.json's serviceDef is what RangerPolarisAuthorizerTest actually authorizes LIST_TABLE_METRICS against, but nothing shipped for operators to register with a real Ranger Admin — so the tested access types and the ones an operator could actually grant could silently diverge. This test asserts the new extensions/auth/ranger/src/main/resources/polaris-ranger-servicedef.json (the artifact I added for operators, referenced from the README) is byte-identical to that fixture's serviceDef, so any future edit to one without the other fails CI instead of drifting unnoticed.

Happy to fold it into RangerPolarisAuthorizerTest as one more @Test method instead of a separate class if you'd rather keep the file count down — let me know which you prefer.

[obelix74]

(Apologies — my previous reply above landed on this thread by mistake; it was meant for the RangerServiceDefConsistencyTest thread. Posting the correct reply there now: #4115 (comment))

[flyingImer]

Spec says metricType is required, enum: [scan, commit], 400 otherwise. Code checks only equalsIgnoreCase("commit"), so any other value including a typo falls to the scan branch and succeeds. I see it as a bug

[obelix74]

Confirmed and fixed in da04347 — metricType is now strictly validated (throws IllegalArgumentException / 400 for anything other than scan/commit), covered by a new test.

[flyingImer]

info: is -1L a safe fallback?

[obelix74]

Agreed, not safe — replaced the -1L sentinel with a checkNotNull precondition in da04347, matching the fail-fast pattern used in IcebergCatalogHandler.

[flyrain]

If we're aiming to have a REST API for metrics queries, I'd suggest moving it to /spec. REST specs should be global for the entire Polaris project, so we shouldn't have separate REST specs for metric queries spread across different extensions.

[flyrain]

+1

[flyrain]

What is the use case of having a principalName filter?

[flyrain]

Do we need catalog/ns/table in the URL path? Hardcoding this hierarchy prevents querying metrics across multiple catalog objects in a single request.
A POST-based endpoint (e.g., accepting a QueryMetricsRequest payload) might be more flexible here, similar to the event query pattern in the IRC event spec (Apache Iceberg PR #12584).

[flyingImer]

The API direction looks right, and the latest POST multi-table shape addresses the API-shape feedback. I don't think this is merge-ready yet. Two contract blockers and two narrower AuthZ/scope questions are inline. Please also rebase onto main and refresh the stacked-PR description before the next round.

[flyingImer]

listMetrics? not sure if reports term makes the most sense here

[flyingImer]

TABLE_READ_DATA currently subsumes TABLE_READ_METRICS here. That lets every data reader query principalName, requestId, and trace/span IDs for all stored reports on the table without an explicit metrics grant. Is that intended? Since this PR adds a dedicated privilege for operational metadata, my bias is to keep it independently grantable unless we explicitly want that disclosure in the TABLE_READ_DATA contract.

[flyingImer]

Maybe I missed this earlier: TABLE_READ_METRICS is grantable on generic tables here, but the API and current ingestion path are Iceberg-only. A generic-table request can authorize successfully and then return no reports, which makes the contract look broader than the implementation. Could we restrict this privilege and the service lookup to ICEBERG_TABLE until generic-table metrics exist?

[flyingImer]

LIST_TABLE_METRICS now requests table-metrics-read, but Polaris pins Ranger 2.9.0 and neither its Polaris service definition nor Ranger master defines that access type. Ranger validates policy access types against the service definition, while this PR adds it only to embedded test fixtures and does not add it to the table resource’s accessTypeRestrictions. Operators therefore cannot grant the new permission through the shipped definition. Could we provide an operator-consumable service-definition update and test LIST_TABLE_METRICS against that exact artifact?

[obelix74]

Addressed in 32d7bf7351d (fix(metrics): ship operator-consumable Ranger service def, close data-read/metrics-read gap):

• Extracted the serviceDef into extensions/auth/ranger/src/main/resources/polaris-ranger-servicedef.json, the artifact operators register with Ranger Admin (README updated with the registration steps).
• Added table-metrics-read to the table resource's accessTypeRestrictions, which was indeed missing.
• Added RangerServiceDefConsistencyTest, which fails the build if that shipped artifact ever drifts from the serviceDef in authz_tests/dev_polaris.json — the same fixture RangerPolarisAuthorizerTest exercises for LIST_TABLE_METRICS and the rest of the authz suite. So the operator-facing service definition and the tested one are now provably the same artifact.

[flyingImer]

I agree with collapsing this to one envelope-based method. The remaining gap is that MetricType and the returned Page are still unrelated values, so a provider can pair COMMIT with scan records and the service has to recover the relationship through casts. Could listMetrics return a sealed QueryResult with typed ScanResult and CommitResult variants, with metricType derived from the variant? That keeps one entry point, makes an invalid envelope fail to compile, and lets the service use an exhaustive switch without casts. One check at the SPI seam can verify that the returned variant matches the requested runtime MetricType.

For example:

sealed interface QueryResult permits ScanResult, CommitResult {
  MetricType metricType();
}

record ScanResult(Page<ScanMetricsRecord> reports)
    implements QueryResult {

  @Override
  public MetricType metricType() {
    return MetricType.S.SCAN;
  }
}

record CommitResult(Page<CommitMetricsRecord> reports)
    implements QueryResult {

  @Override
  public MetricType metricType() {
    return MetricType.COMMIT;
  }
}

QueryResult listReports(
    MetricType metricType,
    long catalogId,
    List<Long> tableIds,
    Long snapshotId,
    Long timestampFrom,
    Long timestampTo,
    PageToken pageToken);

This way will safeguard such:

new CommitResult(scanPage); // compile error

service no longer needs to cast:

QueryResult result = provider.listReports(type, ...);

checkState(
    result.metricType() == type,
    "Provider returned %s for %s",
    result.metricType(),
    type);

return switch (result) {
  case ScanResult scan ->
      toScanResponse(scan.reports());

  case CommitResult commit ->
      toCommitResponse(commit.reports());
};

no op can simply be:

return switch (metricType) {
  case SCAN ->
      new ScanResult(Page.fromItems(List.of()));

  case COMMIT ->
      new CommitResult(Page.fromItems(List.of()));
};

[flyingImer]

IIUC, Ranger and native RBAC give different results for the same conceptual grant: a user with only table-data-read can access table metrics through Ranger, but not through native RBAC. In Ranger, this happens because table-data-read implies table-metrics-read. Could we remove that implication from both table-data-read and table-data-write and add a negative test showing that data access alone does not authorize LIST_TABLE_METRICS?

[obelix74]

Addressed in 32d7bf7351d, same commit as the sibling thread above:

• Dropped table-metrics-read from table-data-read's and table-data-write's impliedGrants in both authz_tests/dev_polaris.json and the intTest fixture, matching the native RBAC side (table-metrics-read is now only implied by table-metadata-full/catalog-content-manage there too).
• Added a policy item granting only table-data-read/table-data-write to a new test principal (dataonly1) in both fixtures.
• Added a negative LIST_TABLE_METRICS test case for dataonly1 (expected isAllowed: false) alongside a positive case for admin1, in tests_authz_table.json.

[dimas-b]

@obelix74 : Do you have time to refresh this PR?

[obelix74]

@obelix74 : Do you have time to refresh this PR?

Working on it.

[dimas-b]

Is this change related to metrics?

[obelix74]

Good catch, not related to metrics at all — that was collateral from making my new RangerServiceDefConsistencyTest's byte-equality check look tidier. Reverted in 5b55c34 (both here and in the intTest fixture / the new polaris-ranger-servicedef.json), back to Polaris (draft).

[dimas-b]

This looks like a spurious change in this PR 🤔

[obelix74]

It's intentional, not merge noise — this is the direct answer to your earlier ask on the sibling thread: #4115 (comment) ("Could we provide an operator-consumable service-definition update and test LIST_TABLE_METRICS against that exact artifact?").

The problem it closes: authz_tests/dev_polaris.json's serviceDef is what RangerPolarisAuthorizerTest actually authorizes LIST_TABLE_METRICS against, but nothing shipped for operators to register with a real Ranger Admin — so the tested access types and the ones an operator could actually grant could silently diverge. This test asserts the new extensions/auth/ranger/src/main/resources/polaris-ranger-servicedef.json (the artifact I added for operators, referenced from the README) is byte-identical to that fixture's serviceDef, so any future edit to one without the other fails CI instead of drifting unnoticed.

Happy to fold it into RangerPolarisAuthorizerTest as one more @Test method instead of a separate class if you'd rather keep the file count down — let me know which you prefer.