(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.

[dimas-b]

Sorry, I missed that 🤦 ... but it was not my ask, it seems 😉

[dimas-b]

Separate class LGTM 👍

[dimas-b]

I see several dev_polaris.json in the source tree 🤔

Would it make sense to refactor Ranger tests to embed polaris-ranger-servicedef.json into the test configs dynamically as opposed to checking for byte-for-byte equality in yet another test?

Perhaps this deserves a separate PR (to be merged before this one)... WDYT?

[obelix74]

Good idea — done in 63b6ac2. RangerServiceDefConsistencyTest is gone; both dev_polaris.json fixtures (unit + IT) are now templates with the serviceDef spliced in from the shipped polaris-ranger-servicedef.json at build time via two new Gradle tasks (generateAuthzTestFixture/generateAuthzItTestFixture), so they can no longer drift by construction instead of by a test catching it after the fact.

While wiring this up I found the IT fixture's serviceDef wasn't actually identical to the shipped one — it carries an extra policyConditions block (the script-based condition evaluator) that only the IT-level sensitivity test (RangerPolicyConditionIT) exercises, and which isn't part of the operator-facing artifact. That's intentional, not drift, so the IT generation task appends that block on top of the shipped serviceDef rather than replacing it — kept in this PR rather than splitting out, since it's small and directly follows from your comment.

[dimas-b]

@obelix74 : did you push the new code?.. I do not see any changes since my last review 😅

[obelix74]

No, last commit was 3 hours ago.

[dimas-b]

The metrics side LGTM 👍

My only concern is with Ranger code changes. Those looks fairly isolated from metrics, so perhaps the bulk of them can be done in a separate PR for the sake of clarity. Metrics-specific changes ("service def") can be added on top and will hopefully result in a smaller "diff"... WDYT?

[dimas-b]

@flyingImer : Does your "request changes" review still hold?

[obelix74]

Split out the general Ranger servicedef-shipping work into #5494, per the suggestion above — traced through the diff and it really was two separable things bundled into one commit:

1. Extracting the (already-existing, test-fixture-only) serviceDef into a shipped operator artifact (polaris-ranger-servicedef.json) plus drift-prevention build machinery — unrelated to metrics, would be needed regardless of this PR. That's all of fix(ranger): ship operator-consumable Ranger service def #5494.
2. The actual metrics-specific delta: adding table-metrics-read to the table resource's accessTypeRestrictions, removing it from table-data-read/table-data-write's impliedGrants, the RangerPolarisOperationSemantics wiring for LIST_TABLE_METRICS, and one negative test case — a few dozen lines.

GitHub won't let this PR's base point at ranger-servicedef-base directly since that branch only exists on my fork, so the diff here won't shrink until #5494 merges — once it does, I'll rebase this PR onto the updated main and it'll drop down to just the metrics-specific delta.

[flyrain]

Thanks @obelix74 for working on it. Post some comments on the spec.

[flyrain]

Could we follow the Iceberg REST Catalog {prefix} convention here, using /{prefix}/metrics/query under the existing metrics API base path, and resolve it through CatalogPrefixParser? That would let clients reuse the prefix returned by IRC configuration instead of requiring the actual Polaris catalog name, and would respect deployments with custom prefix mappings. This would need to use the existing prefix-to-catalog translation in the handler, rather than only renaming the path parameter.

[dimas-b]

Good point about CatalogPrefixParser 👍 I must have missed it 🤦

[obelix74]

Done in e4329a6 — moved to /api/catalog/v1/{prefix}/metrics/query, resolving {prefix} to the Polaris catalog name via CatalogPrefixParser (same pattern as IcebergCatalogAdapter.withCatalog), so custom prefix-to-catalog mappings work correctly here too.

[obelix74]

👍 Landed in e4329a6.

[flyrain]

How do we associate the snapshot ID with a specific table when the request includes multiple tables?

[obelix74]

Restricted snapshotId to single-table requests — the handler now throws 400 if it's set alongside more than one table, since a snapshot ID is scoped to one table and applying it across a multi-table request would silently (and incorrectly) filter every other table's reports too. Kept it as a single top-level scalar rather than moving it per-table into TableRef, to avoid a matching MetricsQuerySpi/JDBC persistence-layer change (that provider is already implemented in the stacked JDBC PR). Done in e4329a6.

[flyrain]

Do we need to give a default value in the spec?

[obelix74]

Good catch — the spec's default: 100 didn't actually match server behavior: when both pageSize and pageToken are omitted, PageToken.build falls through to READ_EVERYTHING (unbounded), same as every other paginated Polaris endpoint using that utility, not 100. Removed the misleading default in e4329a6 rather than special-casing this endpoint to actually default to 100.

[flyrain]

The metrics report is under a specific catalog, and we don't allow cross catalog metrics query or non-catalog related metrics(e.g., role/principal) query. Could we reuse the existing Iceberg REST Catalog API base path and expose this as POST /api/catalog/v1/{prefix}/metrics/query, instead of introducing /api/metrics-reports/v1? The multi-table query needs a new operation, but it can remain a Polaris extension under the existing catalog API, with {prefix} resolved through CatalogPrefixParser.

[obelix74]

Same fix — done in e4329a6, same reasoning as the reply below on the base-path comment.

[flyrain]

Could we use one ListMetricsResponse with a common report envelope and an opaque JSON payload, instead of specializing the list responses and duplicating IRC's scan/commit metric fields? The envelope could expose metricType, table (namespaces and name), timestampMs (server receipt time), and optional snapshotId, with optional actor/request context. The payload would preserve the original Iceberg report as a JSON object (type: object, additionalProperties: true), allowing new metric fields to pass through without changing this API's schema. This would also require preserving the full report through ingestion and persistence, rather than reconstructing it from a fixed set of fields.

Do we need a public report id for this query-only API? Unless there is a concrete deduplication or individual-report lookup use case, I would omit it from the required envelope. An internal unique key for pagination can remain inside the opaque page token; table, timestamp, and snapshot ID should not be assumed to uniquely identify a report.

[obelix74]

Done in e4329a6 — collapsed to a single ListMetricsResponse/MetricsReport with the envelope you described (metricType, table, timestampMs, optional snapshotId/actor/request) and an opaque payload: object (generates as Map<String, Object>) instead of the typed Scan/CommitPayloadData schemas. Also dropped the id field per your note — agreed there's no lookup-by-id use case for a query-only API, and the opaque page token can carry whatever internal key pagination needs.

One thing scoped narrower than what you described: MetricsPersistence (the write-path SPI) already merged as part of PR0 (#5068), so it stores a fixed set of typed columns rather than the raw report JSON. For this PR, payload is still reconstructed from those persisted typed fields rather than threaded through unmodified from ingestion — genuinely preserving the full original report end-to-end would mean reopening the already-merged SPI and the stacked JDBC PR's persistence layer, which felt like it deserved its own follow-up rather than riding on this one. The response contract is opaque/forward-compatible starting now either way.

[flyingImer]

The opaque schema gives us room to add fields, but it doesn't make the current payload a preserved original report. The service reconstructs a selected set of fields and omits even the metadata already stored with the record. If full preservation is deferred, could we describe this as a reconstructed projection and remove the original-report guarantee from the spec? A populated scan/commit response test would also make the current boundary explicit.

[flyingImer]

@dimas-b The earlier envelope and Ranger concerns are addressed. I'm still requesting changes for the remaining management-API grant gap, with narrower contract corrections noted in the review.

I'm also revising my earlier 501 position: NoOp is a valid query implementation, so an empty 200 is right. The spec, SPI Javadoc, and CHANGELOG should reflect that.

After #5494 lands, please rebase and rerun CI. Main already uses privilege code 103 for SEMANTIC_MODEL_LIST, so TABLE_READ_METRICS needs an unused code.

[flyingImer]

This privilege is defined internally, but the management API's TablePrivilege, NamespacePrivilege, and CatalogPrivilege enums do not include it. Grant requests use those generated enums, so an operator cannot grant metrics-only access through the normal API and must use a broader privilege such as TABLE_FULL_METADATA. Could we expose TABLE_READ_METRICS in the applicable management enums and add a grant/list/revoke round-trip test?

[obelix74]

Added TABLE_READ_METRICS to TablePrivilege, NamespacePrivilege, and CatalogPrivilege in the management API spec, and added a grant/list/revoke round-trip integration test (testGrantListRevokeTableReadMetrics). Pushed.

[dimas-b]

Good point. Unfortunately, this is a fresh API change. I think we have to make a separate note of that on the dev list according to Polaris processes.

[dimas-b]

https://lists.apache.org/thread/c5jq95qwzn5dtc103rzk457gr8r0d6zh

[dimas-b]

LGTM 👍

[dimas-b]

nit: is this just a space change? Is it avoidable?

[dimas-b]

Good point. Unfortunately, this is a fresh API change. I think we have to make a separate note of that on the dev list according to Polaris processes.

[dimas-b]

The dev email about Management API changes is did not draw attention. I assume we're ok to proceed by lazy consensus.

https://lists.apache.org/thread/c5jq95qwzn5dtc103rzk457gr8r0d6zh

https://lists.apache.org/thread/l5z6jkxlnsxsptp0mvlswohyb2v0ctqo

@flyingImer : WDYT?

[flyingImer]

@dimas-b No objection from my side to the management API addition. To make the remaining scope explicit, these earlier concerns are addressed:

• Management API grant support, including the grant/list/revoke test.
• The single, type-safe SPI envelope.
• The operator-facing Ranger service definition and removal of metrics access from data-read/write grants.

Before merging, only the two documentation corrections remain from my review:

• NoOp behavior: empty 200 is correct. Please update the spec, SPI Javadoc, and CHANGELOG that still promise 501.
• Payload fidelity: please describe the reconstructed projection instead of promising preservation of the original report.

Full original-report preservation can remain a follow-up. With those descriptions corrected, I have no remaining blocking concern.

[dimas-b]

@flyingImer @obelix74 : based email discussion, shall we move the Management API changes into a separate PR (to be merged after this PR)? Missing grants may be an inconvenience, but admin users should still have access, while we sort out lower-level grants 🤔 WDYT?

https://lists.apache.org/thread/l5z6jkxlnsxsptp0mvlswohyb2v0ctqo

[dimas-b]

@flyingImer : just to sync up on the Noop case and 501 - this is the place where you suggest to return 200 / Empty response if metrics persistence is not available, right?

That matches current java code, as far as I can tell, so we're only talking about making the spec reflect current code behaviour, right?

[obelix74]

Thanks @flyingImer. Both documentation corrections are in 67752cb:

• NoOp behavior: the spec no longer promises 501 (the description now says an empty reports list with 200, and the 501 response is removed); the MetricsQuerySpi Javadoc and the CHANGELOG entry say the same.
• Payload fidelity: the spec, CHANGELOG, and MetricsReportsService Javadoc describe payload as a projection reconstructed from the persisted record, and no longer claim it preserves the original report. I also added populated scan and commit response tests that pin down that boundary (for example, record metadata is not included in the payload).

I also rebased onto current main, reallocating TABLE_READ_METRICS to code 110. The one remaining CI failure, PolarisAdminServiceTest.testRotateCredentialsConcurrentModificationThrowsCommitConflictException, also fails on main itself; #5637 fixes it.