Conversation
| **Response:** | ||
| ```json | ||
| { | ||
| "nextPageToken": "eyJ0cyI6MTcwOTMzNzYxMjM0NSwiaWQiOiI1NTBlODQwMCJ9", |
There was a problem hiding this comment.
Let's make sure that any potential Events APIs are in line with the Iceberg Events API documentations. The Iceberg Events API is close to consensus and we should ensure that we do not merge anything that would not be close-to-future spec incompatible.
There was a problem hiding this comment.
There was a problem hiding this comment.
Pushed a commit to bring this in sync with the PR you linked above.
| |--------|------|-------------| | ||
| | GET | `/api/management/v1/catalogs/{catalogName}/events` | List events for a catalog | | ||
| | GET | `/api/management/v1/catalogs/{catalogName}/events/{eventId}` | Get a specific event | | ||
| | GET | `/api/management/v1/catalogs/{catalogName}/namespaces/{namespace}/tables/{table}/scan-metrics` | List scan metrics for a table | |
There was a problem hiding this comment.
/api/management appears to be the prefix for the API that "changes" the Polaris Catalog and RBAC data at user's requests.
Scan reports appear to be about accessing pre-populated records without "managing" anything.
In other words a server that does not support end users to manage catalogs, may still expose scan metrics (etc.) for access.
WDYT about /api/metrics-reports/v1/... and /api/events/v1/... for events?
There was a problem hiding this comment.
+1 My initial though too was that /management API is the right place for these endpoints, but moving it to /events and /metrics-reports instead is more convincing indeed.
There was a problem hiding this comment.
@dimas-b and @nandorKollar Please see https://docs.google.com/document/d/1WtIsNGVX75-_MsQIOJhXLAWg6IbplV4-DkLllQEiFT8/edit?tab=t.0 and apache/iceberg#12584.
Looks like the Iceberg events API is /v1/{prefix}/events:. I have updated the markdown to be in sync. Is this ok?
There was a problem hiding this comment.
Thanks, I wasn't aware of the aforementioned doc. Since events will hopefully be part of Iceberg REST spec, /api/catalog/v1/{prefix}/events sounds right for me. Nonetheless, the other two endpoints are still questionable for me, I'd rather take @dimas-b 's recommendation, and use /metrics-reports instead.
There was a problem hiding this comment.
@nandorKollar ah yes, missed that. Pushed a commit with that change.
There was a problem hiding this comment.
It's totally fine for Polaris to support the events endpoint from the IRC spec when it is approved.
However, we should not assume a particular IRC spec ahead of time.
If Polaris is to expose events before any similar endpoint is defined in IRC, I believe Polaris should use a spec of its own.
With that in mind /api/catalog is the prefix for the IRC API in Polaris. I do not think it would be wise to reuse it for Polaris' own endpoints.
There was a problem hiding this comment.
@dimas-b so you prefer /api/events/v1 correct? @adnanhemani are you ok with this?
There was a problem hiding this comment.
yes (/api/events/v1), I believe it is best to avoid URI path clashes with IRC events API until that spec is approved.
|
|
||
| | Endpoint | Required Privilege | Scope | | ||
| |----------|-------------------|-------| | ||
| | List/Get Events | `CATALOG_MANAGE_METADATA` | Catalog | |
There was a problem hiding this comment.
"Manage" access may be too broad a grant for someone who only needs to "read" events. Why not add a new permission?
... same for reading scan/commit metrics?
There was a problem hiding this comment.
I've updated the proposal to introduce dedicated read-only privileges:
- CATALOG_READ_EVENTS - for read-only access to catalog events
- TABLE_READ_METRICS - for read-only access to scan/commit metrics
|
|
||
| ### 5.2 Rationale | ||
|
|
||
| - **Events** contain catalog-wide audit information and should require catalog-level administrative access |
There was a problem hiding this comment.
What about separation of duties? Do all people who are authorized to "see" events have to be authorized to make changes to (manage) the catalog?
There was a problem hiding this comment.
I've updated the proposal to address this:
- A new CATALOG_READ_EVENTS privilege is introduced specifically for read-only event/audit access
- CATALOG_MANAGE_METADATA will imply CATALOG_READ_EVENTS (so admins can still see events)
- But CATALOG_READ_EVENTS does not imply management access
This allows security auditors or compliance tools to read events without having any catalog management capabilities.
| ### 5.2 Rationale | ||
|
|
||
| - **Events** contain catalog-wide audit information and should require catalog-level administrative access | ||
| - **Metrics** are table-specific and align with read access since they describe query patterns and commit history |
There was a problem hiding this comment.
I agree that whoever can read data, should be able to read scan/commit reports. However, is the reverse true? I suppose separation of duties applies here too. For example, a tool that reads those reports does not necessarily have to be authorized to read tables. WDYT?
There was a problem hiding this comment.
I've updated the proposal:
- A new TABLE_READ_METRICS privilege is introduced for metrics-only access
- TABLE_READ_DATA will imply TABLE_READ_METRICS (users who can read data can see metrics about their queries)
- But TABLE_READ_METRICS does not imply data access
site/content/in-dev/unreleased/proposals/observability-rest-api.md
Outdated
Show resolved
Hide resolved
I sent an email last evening (or so I thought), it was sitting in my Outbox (MS Exchange!) till now. |
Updates the Observability REST API proposal to align with the emerging Iceberg Events API specification (apache/iceberg#12584) per review feedback. Key changes: - Changed Events endpoint from GET to POST with request body - Moved Events API to Iceberg REST Catalog path (/api/catalog/v1/{prefix}/events) - Adopted Iceberg event structure (event-id, request-id, timestamp-ms, operation) - Added standard operation types (create-table, update-table, drop-table, etc.) - Added Polaris custom operation types with x-polaris-* prefix convention - Updated OpenAPI schemas for Iceberg compatibility - Added Section 8 documenting Iceberg alignment rationale - Added mapping table from Polaris internal events to Iceberg operations References: - Iceberg Events API PR: apache/iceberg#12584 - Iceberg Events API Doc: https://docs.google.com/document/d/1WtIsNGVX75-_MsQIOJhXLAWg6IbplV4-DkLllQEiFT8
Updates the Authorization section to address separation of duties concerns: - Replace CATALOG_MANAGE_METADATA with new CATALOG_READ_EVENTS privilege - Replace TABLE_READ_DATA with new TABLE_READ_METRICS privilege - Add detailed rationale for separation of duties - Document privilege hierarchy (higher privileges imply lower ones) - Add implementation notes for new privileges This ensures: - Read-only audit access does not require management permissions - Monitoring tools can access metrics without requiring data read access - Fine-grained access control is possible for different operational roles
Address reviewer feedback to clarify that OpenAPI YAML should be in separate files upon approval for ease of processing.
Address reviewer feedback that metrics reports are read-only access to pre-populated data, not catalog management operations: - Change base path from /api/management/v1/ to /api/metrics-reports/v1/ - Update all example requests with new path - Specify new spec file: spec/metrics-reports-service.yml - Update Files to Modify section with new service locations - Add rationale explaining separation from management API This allows servers that don't support catalog management to still expose metrics reports for monitoring and observability.
| discriminator: | ||
| propertyName: operation-type | ||
| mapping: | ||
| create-table: "#/components/schemas/CreateTableOperation" |
There was a problem hiding this comment.
We should think about how to map Polaris event types (PolarisEventType enum values) to these operation-types. In Polaris we separate events to before and after operation (BEFORE_CREATE_CATALOG and AFTER_CREATE_CATALOG) how can we map these to an operation-type? Or they're independent, should we use completely separate events here? Probably everything fits in CustomOperation, but if possible, we should use these standardised event types, though in many cases it isn't possible, role and principal related events can only be mapped to CustomOperation.
There was a problem hiding this comment.
@nandorKollar I took a pass on this in a separate commit. I am most likely wrong, but let me know what you think.
Address review comment from nandorKollar regarding how Polaris internal event types (BEFORE_*/AFTER_*) map to Iceberg Events API operation-types. New section 7.5 covers: - Design decision: Only AFTER_* events exposed (completed operations) - Complete mapping table from PolarisEventType to operation-type - Standard Iceberg ops map directly, Polaris-specific use x-polaris-* prefix - Read-only operations (GET/LIST) are excluded - Implementation guidance with event filtering code sample
|
Couple of more higher-level thoughts:
|
@dimas-b would you be ok with a single @adnanhemani this is the 3rd proposal I am writing and I find markdown and github better for a collaboration perspective. I can reason about my changes as can the person who asks the changes. In proposals that grow quite big, I could barely keep up. The cognitive effort to make sure all comments were addressed was very high. |
I do not think any objections to that "in principle". Let's continue this discussion on the specific OpenAPI spec text / diff ... I have not looked thought it end-to-end yet :) |
Address feedback from adnanhemani to use a single metrics endpoint with
metricType as a query parameter instead of separate /scan-metrics and
/commit-metrics endpoints.
Changes:
- Single endpoint: /catalogs/{catalogName}/namespaces/{namespace}/tables/{table}/metrics
- Required query parameter: metricType (enum: scan, commit)
- Unified response schema: ListMetricsResponse with metricType discriminator
- Future-extensible: new metric types can be added without new endpoints
- operation parameter only applicable when metricType=commit
The base path /api/metrics-reports/v1/ already establishes the context,
so the trailing /metrics is redundant.
Before: /api/metrics-reports/v1/catalogs/{catalog}/namespaces/{ns}/tables/{table}/metrics
After: /api/metrics-reports/v1/catalogs/{catalog}/namespaces/{ns}/tables/{table}
Pushed a commit with this change. |
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
LGTM with just a few minor comments
|
|
||
| | Custom Operation Type | Description | | ||
| |----------------------|-------------| | ||
| | `x-polaris-create-catalog-role` | Catalog role created | |
There was a problem hiding this comment.
I believe the x- prefix is mostly obsolete these days... HTTP headers no longer promote its use, AFAIK.
The polaris- prefix alone is probably sufficient to disambiguate.
| - **Operation-centric model**: Events are structured around operations (create-table, update-table, etc.) | ||
| - **Custom extensions**: Support for `x-` prefixed custom operation types for Polaris-specific events | ||
|
|
||
| #### Request Body (`QueryEventsRequest`) |
There was a problem hiding this comment.
just to confirm: this matches the Iceberg events API proposal, right?
There was a problem hiding this comment.
The request body properties (continuation-token, page-size, after-timestamp-ms, operation-types, catalog-objects-by-name, catalog-objects-by-id, object-types, custom-filters) already match the Iceberg Events API proposal from the Google Doc.
| - For metricType=scan: ScanMetricsReport objects | ||
| - For metricType=commit: CommitMetricsReport objects | ||
| items: | ||
| oneOf: |
There was a problem hiding this comment.
I believe this definition allows each array element to be either a scan report or a commit report and the type is independent of other elements.... or am I mistaken?
There was a problem hiding this comment.
WDYT about making ListMetricsResponse polymorphic at the top level?.. or at least in the property the contains report data?
There was a problem hiding this comment.
Thank you! When I replaced the two endpoints with one, this broke. I followed your suggestion for ListMetricsResponse - that is an elegant solution. Thank you.
site/content/in-dev/unreleased/proposals/observability-rest-api.md
Outdated
Show resolved
Hide resolved
|
|
||
| --- | ||
|
|
||
| ## 3. Design Principles |
There was a problem hiding this comment.
nit: maybe briefly mention that the new API will process Polaris realms the same way as the existing APIs
Changes based on reviewer comments:
1. Use /api/events/v1/{prefix} instead of /api/catalog/v1/{prefix}/events
to avoid URI path clashes with Iceberg REST Catalog events API until
that spec is approved (dimas-b)
2. Change x-polaris-* to polaris-* for custom operation types since the
x- prefix is obsolete (dimas-b)
3. Add note confirming QueryEventsRequest matches Iceberg Events API
specification for ecosystem compatibility (dimas-b)
4. Update spec file references from rest-catalog-open-api.yaml to new
events-service.yml
Address dimas-b feedback: the previous oneOf at item level incorrectly allowed mixing scan and commit reports in a single response. Now using discriminator at response level: - ListMetricsResponse is polymorphic with metricType discriminator - ListScanMetricsResponse: metricType=scan, reports array of ScanMetricsReport - ListCommitMetricsResponse: metricType=commit, reports array of CommitMetricsReport This correctly expresses that all items in a response are of the same type.
Address dimas-b feedback: mention that the new APIs process Polaris realms consistently with existing APIs.
|
I think the persistence layer is a precondition for this API, it seems that we don't persist most of the events? @dimas-b do you happen to know, am I looking the right point where we persist the events? |
Address nandorKollar's observation that PolarisPersistenceEventListener currently only persists AFTER_CREATE_TABLE and AFTER_CREATE_CATALOG events. Added new section 7.1 documenting: - Current state: only 2 event types are persisted - Required changes: list of all AFTER_* mutation events to add - Implementation approach: phased rollout (Iceberg ops, then Polaris-specific) This is a prerequisite for the Events API to be useful.
You are correct. I added a listener that extends InMemoryBufferEventListener and persists many more events: Table Operations: Namespace Operations: View Operations: Catalog Operations: I have not persisted principal / role management, policy events and a few others (I should add them). Adding a section to the proposal highlighting this. |
|
|
||
| #### Implementation Approach | ||
|
|
||
| 1. **Phase 1**: Extend `PolarisPersistenceEventListener` to handle all Iceberg-standard operations (tables, views, namespaces) |
There was a problem hiding this comment.
I am not sure if this should be made a Polaris standard implementation. Users can implement their own listeners to persist the kind of events they are interested in.
There was a problem hiding this comment.
Definitely what to persist is outside the scope of this API proposal, which is about how to retrieve what has been persisted :)
Did you already extend the |
|
Event persistence was added in #1844 , I think... was something missing? |
|
|
||
| | Operation Type | Description | | ||
| |----------------|-------------| | ||
| | `create-table` | Table created and committed | |
There was a problem hiding this comment.
Mapping these type code from PolarisEventType is non-trivial. Polaris has before/after events and exact names do not match. Could you explicitly list how we will deduce API-level type codes from the java enum?
There was a problem hiding this comment.
TBH, this may be a bit of an obstacle to supporting the "expected" Iceberg events API spec.
I would not mind having Polaris-specific type codes for now and dealing with the IRC events API when it gets approved. WDYT?
There was a problem hiding this comment.
@dimas-b I think there's a section in the doc which talks about this mapping: https://github.com/apache/polaris/pull/3924/changes#diff-4067fb5547bab712380d93507445f600e2ab164380e5cbe5a8e54011289f0d65R1211
Before events are not taken into account, which sounds like a reasonable choice for me. The proposal in the Iceberg spec has a Custom event category, I think if we need to list BEFORE* events too, then we can use that.
There was a problem hiding this comment.
ok, I'll leave it up to you then :)
From my personal POV, it would be preferable to have a "solid" set of event types based on current Polaris code than hypothetical types from an in-progress spec... but I do not mind using the proposed types from IRC as long as we're prepared for spec changes here.
A side benefit to using Polaris type codes is that the data in the database, in events SPI, and REST API would use the same type codes. It could be significant in deployments that use several of those sources, but I do not have a concrete use case in mind.
There was a problem hiding this comment.
Event serialization (JSON) would also be easier to handle with Polaris's own data structures and type codes.
There was a problem hiding this comment.
The mapping is: only AFTER_* events are exposed via the API (the BEFORE_* events are internal and not persisted to the EVENTS table). The translation from Java enum to API-level type code strips the AFTER_ prefix and lowercases with hyphens — e.g., AFTER_CREATE_TABLE → create-table, AFTER_DROP_TABLE → drop-table. I'll add an explicit mapping table to the proposal to make this unambiguous.
oh, I did not commit it to Polaris, for our deployment, we depend on runtime jars and we build our own distribution. I added my custom event listener (which was also creating new prometheus metrics). Please take on that subtask. That will help. |
yes, but only two events were persisted. Looking at PolarisPersistenceEventListener.java, the onEvent switch only handles:
|
Yes, but seems that the event lister implementation, which could be the bridge between the event source and the persistence layer handles only a handful events. Or everyone is supposed to write it's own event lister to call the persistence layer and to map the right lifecycle events to event entities? |
|
I think we should also think about catalog federation too. I think we should not federate events, Polaris should be the source of truth for each event (details about federation are documented here: https://docs.google.com/document/d/1Q6eEytxb0btpOPcL8RtkULskOlYUCo_3FLvFRnHkzBY/edit?tab=t.0) both in IRC and HMS federation scenarios. |
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
The PR LGTM as a proposal. I suppose specific API PRs are still going to be open for further comments and nitpicking when an end-to-end OpenAPI yaml is created :)
This is to say that from my POV the approach outlined in this proposal is good to proceed with.
If other reviews what to take more time on this PR, it's fine too, from my POV.
| |-----------|------|----------|---------|-------------| | ||
| | `metricType` | string | **Yes** | - | Type of metrics to retrieve: `scan` or `commit` | | ||
| | `pageToken` | string | No | - | Cursor for pagination | | ||
| | `pageSize` | integer | No | 100 | Results per page (max: 1000) | |
There was a problem hiding this comment.
IIRC, the IRC API uses hyphenated param names, like page-size. WDYT about using the same pattern here?
nit: I personally just like hyphenated query param names too. I think there are nice on curl CLI 😉
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
I assume this proposal is limited to events stored in Polaris. In general (future), if Polaris federates catalogs, why would we not want to federate events (which are going to the part of the catalog API in Iceberg)?.. but this is a different topic, probably worth a separate discussion 🤔 |
Totally agree, this should be a separate discussion, it is out of scope for this proposal. I told that we shouldn't federate the events, because I though that they are considered 'local' for a given catalog. Both local and remote catalog track the events independently, but it probably makes more sense to federate events too. |
|
@flyrain As discussed in the mailing list, can you please review this? Since there are other reviews already, I prefer to stay with markdown. I will be happy to create a google doc for posterity once you are happy here. Again, as discussed, even if I depend on just the |
| "principal": "security-admin@example.com" | ||
| }, | ||
| "operation": { | ||
| "operation-type": "custom", |
There was a problem hiding this comment.
I wonder if this example is mixing two scopes a bit. polaris-grant-privilege looks more like a realm/admin event than a catalog-scoped one, but the API here is catalog-scoped (/api/events/v1/{prefix}). Should the proposal clarify whether these realm-level admin events are intentionally in scope for this API, or whether they should be handled separately? It feels worth making explicit before the contract settles.
There was a problem hiding this comment.
For v1, /api/events/v1/{prefix} is intended to be strictly catalog-scoped, returning events for changes to catalog objects (namespaces/tables/views) within that catalog. I updated the proposal to make that scope explicit and adjusted the custom-operation example to only use catalog-scoped operations (removed realm/admin examples like credential rotation). Also aligned the custom operation type naming so the OpenAPI schema and examples consistently use the polaris-* prefix.
Note that @dimas-b suggested we remove the x- prefix earlier.
There was a problem hiding this comment.
Right, AFAIK, even HTTP does not require the x- prefix for custom headers now.
| - drop-namespace | ||
| - $ref: '#/components/schemas/CustomOperationType' | ||
|
|
||
| CustomOperationType: |
There was a problem hiding this comment.
nit: this schema still requires custom operation types to start with x-, but the proposal/examples seem to have moved to polaris-.... I feel we should align those before implementation so the schema matches the examples in the doc.
| schema: | ||
| type: integer | ||
| format: int64 | ||
| - name: operation |
There was a problem hiding this comment.
IIUC, it seems operation behavior is explicit to metricType=scan
I wonder if this is starting to push Polaris a bit too far into the role of a full metrics query system. Once we get into more and more type-specific filters and query semantics here, it feels like we may be moving toward something closer to a built-in observability backend.
My bias would be for Polaris to support a smaller set of community-recognized built-in metrics well, and provide good SPI/extensibility points for systems that want richer querying / visualization / use-case-specific metrics, rather than trying to make Polaris itself cover all of those cases.
There was a problem hiding this comment.
Fair enough. I’ve tightened the proposal by removing the type-specific operation query filter and adding explicit v1 non-goals (no aggregation/group-by, no derived computations / query language). The intent is paged retrieval of persisted reports with basic filters, with richer analysis expected via export/sinking to an external observability system.
| ```yaml | ||
| components: | ||
| schemas: | ||
| ScanMetricsReport: |
There was a problem hiding this comment.
2 cents: not sure I'm a big fan of this style - fully flattened schema, and closely tied to Iceberg spec/impl, making such interface super fragile. Furthermore, I feel the metrics interface should be generic as well - simply mapping source to a new type, nested payload by default, probably with some version metadata as scaffolding for iceberg metrics schema determination, but defining SPI for custom transforms (like flattening)
Following the same rationale, I feel the metrics persistence layer is suboptimal as well
There was a problem hiding this comment.
I share the concern about fragility here. The flattened schemas in this draft are mostly meant as an illustrative “concrete shape” based on what we persist today, but I agree that a tight 1:1 mapping to Iceberg internals (and especially fully flattening) risks locking us into a contract that will churn as Iceberg evolves.
I’d like to treat the long flattened schema as non-normative for now and move the proposal toward a more generic contract (stable envelope + versioned payload), with optional/derived fields if we ever decide they’re worth standardizing. On the persistence side: also agree the current storage model may need a follow-up proposal; for this one I’m trying to stay limited to “expose what’s already being captured,” but I’ll call out the persistence design tradeoffs more explicitly.
There was a problem hiding this comment.
(commenting in general) From my POV Polaris does not have to "bind" to Iceberg APIs or types.
If it makes sense to structure the payload differently, I'd support developing a Polaris-specific API for accessing these reports.
Same for events: even if we want to support the IRC events endpoint when it is available, Polaris can still have a separate API for accessing events.
I personally do not have any particular preference about how the payloads are defined exactly, as long as they are parseable using the usual tools.
+1 to stable envelope.
There was a problem hiding this comment.
Let me know what you think.
Stable envelope (recommended for both Events + Metrics)
To reduce coupling to any single upstream schema (e.g., Iceberg) and keep client integrations resilient, the Observability APIs SHOULD return records using a stable envelope: a small, Polaris-owned set of top-level fields that remain consistent across all event/report types, plus a versioned payload for the type-specific body.
The envelope enables clients to reliably paginate, deduplicate, and correlate records (request IDs / trace IDs) without needing to understand the full payload schema. Type-specific details (Iceberg-aligned or Polaris-specific) live under payload, identified by payload.type and payload.version.
Envelope fields (conceptual):
• id (unique identifier)
• timestampMs (event/report time)
• catalog / realm (if applicable)
• actor (principal/service + optional client metadata)
• request (requestId, trace/span ids)
• object (resource identity: namespace/table/view identifiers and optional UUID/snapshotId)
• payload: { type, version, data, extensions? }
Compatibility / evolution rules
• Envelope is additive-only: new envelope fields may be added as optional; existing envelope fields MUST NOT change meaning or type, and MUST NOT be removed.
• Breaking changes require versioning at the API boundary: any breaking envelope change requires a new major API version (e.g., /v2/...) rather than changing /v1.
• Payload is independently versioned: payload.type selects the schema family (e.g., iceberg.events, iceberg.metrics.scan, polaris.events), and payload.version increments on breaking payload changes.
• Unknown payloads are safe to ignore: clients MUST treat unknown payload.type or higher payload.version as opaque and continue to operate using envelope fields (e.g., still paginate / display metadata).
• Payload changes within a version are additive-only: within a given payload.type + payload.version, new fields may be added as optional; removals/renames/type changes require a new payload.version.
• Flattening is a presentation choice: the default representation SHOULD keep domain-specific structures nested under payload.data; alternative “flattened” views (if needed) should be offered as an explicit, separately-versioned representation rather than redefining the canonical schema.
|
|
||
| #### Implementation Approach | ||
|
|
||
| 1. **Phase 1**: Extend `PolarisPersistenceEventListener` to handle all Iceberg-standard operations (tables, views, namespaces) |
There was a problem hiding this comment.
2 cents: I wonder if the proposal would benefit from evaluating the Events API and Metrics API a bit more independently. The Events side feels relatively close to Polaris' catalog/change-log scope (especially Iceberg side has not yet settled), but the Metrics side seems more likely to pull Polaris toward a broader observability product surface. If that distinction is real, would it make sense to either scope the built-in metrics surface down more aggressively, or frame extensibility / sink-export as the primary path for richer metrics use cases?
There was a problem hiding this comment.
I bundled them because (a) both are read-only query surfaces over data Polaris already persists and (b) they share the same core concerns (RBAC, pagination, filtering, realm handling) and (c) I need both as I am rolling Polaris out to production.
But I think it would be clearer to evaluate/scope them independently:
• Events API: stays close to core catalog/change-log behavior, and I’m explicitly trying to track the Iceberg Events API direction (while it’s still settling).
• Metrics API: I don’t want this to imply Polaris is becoming a general “observability platform”. I can tighten the framing/scope to “authorized access to already-recorded scan/commit reports for debugging/forensics/basic monitoring”, and explicitly position export/sink integration (OTel/Prometheus/Kafka/etc.) as the path for richer/aggregated metrics use cases rather than expanding the built-in REST surface.
There was a problem hiding this comment.
From my POV, Polaris is a platform, indeed. In this sense, I think it is critical to enable users to control what features are at play in runtime, since different users have different use cases. This is why I originally advocated for isolating Metrics Persistence from MetaStore Persistence.
If a user decides to leverage the "native" Polaris (scan/commit) Metrics Persistence, I do not see any disadvantage in also exposing an (optional) REST API for loading these metrics from Polaris Persistence.
The degree of support and sophistication that goes into this sub-system is up to the community. If we have contributors (like @obelix74 ) who are willing to evolve it, I see no harm in some functionality overlap with more focused metrics platforms. Again, the key point is for all users to have control and be able to opt in or out of this feature in their specific deployments.
Of course, offloading scan/commit metrics storage to a specialized observability system is possible too (assuming someone develops integration code for that, which is very welcome).
|
|
||
| ### 7.5 Pagination Token Format | ||
|
|
||
| Internal format (base64-encoded JSON, opaque to clients): |
There was a problem hiding this comment.
Current Poalaris page tokens use SMILE.
| under the License. | ||
| --> | ||
|
|
||
| # Proposal: REST API for Querying Table Metrics and Events |
There was a problem hiding this comment.
Why do we combine event and metrics endpoint together? I'd suggest to break this to two proposals; one for events and one for metrics.
As I said in the dev mailing thread, the IRC event endpoint is WIP in the Iceberg community. We mighty wait until it is finalized to avoid inconsistency.
There was a problem hiding this comment.
@flyrain Responded here. #3924 (comment). Also see #3924 (comment). Both have intimate knowledge of Iceberg, please let me know if that proposal (envelope + payload) gives you a path forward.
| - Trace operations via `request_id` | ||
|
|
||
| ### 2.5 Audit & Compliance | ||
| - Track who created/dropped/modified catalog objects |
There was a problem hiding this comment.
tbh I'm not sure this use case can be fulfilled with the present proposal where events are assumed to be stored in the database. For a few reasons:
- The JDBC event listener may not have been activated.
- The
eventstable may need continuous pruning to avoid uncontrollable growth. IOW it may not be possible to achieve the desired retention period with this.
These operational constraints are imho incompatible with audit & compliance use cases.
| | POST | `/api/events/v1/{prefix}` | Query events for a catalog | | ||
| | GET | `/api/metrics-reports/v1/catalogs/{catalogName}/namespaces/{namespace}/tables/{table}` | List metrics for a table (type specified via query parameter) | | ||
|
|
||
| > **Note:** The Events API uses a dedicated `/api/events/v1/` namespace to avoid URI path clashes with the [Iceberg Events API specification](https://github.com/apache/iceberg/pull/12584) until that spec is approved. The API design follows Iceberg Events API patterns for future compatibility. The metrics API uses a dedicated `/api/metrics-reports/v1/` namespace since it exposes pre-populated records rather than managing catalog state. |
There was a problem hiding this comment.
However, I fear this will complicate the implementation significantly: you will need a separate OpenAPI YAML, which means a separate Gradle API module, separate REST resource and separate service stub. Then when it's time to use the "official" endpoint, all the code will have to be moved, and the endpoint implementation will end up in IcebergCatalogHandler.
Alternatively, couldn't we introduce the endpoint directly in the IRC spec and mark it experimental/beta until it graduates on the IRC side?
|
|
||
| | Use Case | Required Privilege | Why Not Reuse Existing? | | ||
| |----------|-------------------|------------------------| | ||
| | Security auditor reviewing catalog changes | `CATALOG_READ_EVENTS` | Should not require `CATALOG_MANAGE_METADATA` (management access) | |
There was a problem hiding this comment.
Isn't this a bit coarse-grained? This persona has access to the entire catalog's events. I think it could be interesting to define a persona that has access only to events of a certain namespace or table, wdyt?
Also, I would expect someone that has TABLE_READ_DATA on some table to be able to see events pertaining to that table. Similar to your "Data analyst with table access" persona below.
It would also be good to mention here the use case of catalog federation / synchronization.
| CATALOG_MANAGE_METADATA | ||
| └── CATALOG_READ_EVENTS (implied) | ||
|
|
||
| TABLE_FULL_METADATA / TABLE_READ_DATA |
There was a problem hiding this comment.
Are we sure about TABLE_FULL_METADATA? This privilege is a super-privilege that subsumes many privileges related to table metadata updates, but not to table data reading or writing.
Moreover, the privileges required to report metrics today are:
REPORT_READ_METRICS(TABLE_READ_DATA),
REPORT_WRITE_METRICS(TABLE_WRITE_DATA),
Maybe we should do something similar and require TABLE_READ_DATA for retrieving scan metrics and TABLE_WRITE_DATA for retrieving commit metrics. Wdyt?
| The new privileges fit into the existing hierarchy as follows: | ||
|
|
||
| ``` | ||
| CATALOG_MANAGE_METADATA |
There was a problem hiding this comment.
FYI in #3927 there is a new privilege being introduced: CATALOG_READ_DATA. I think it should subsume CATALOG_READ_EVENTS as well.
|
|
||
| #### Required Changes | ||
|
|
||
| The persistence layer needs to be extended to capture all `AFTER_*` mutation events that should be exposed via the Events API: |
There was a problem hiding this comment.
That's where I think we are being too narrow-minded since the beginning about events.
We cannot / should not predict what events are going to be used for.
If you don't want to expose the before events in the REST API, fine: I hear your arguments and I'm OK with it. But other users may have another usage for before events, and may desire to see them persisted in the database and/or showing up in CloudWatch. Other users may decide that they want to persist just the after table events but not the after catalog ones, etc.
I would prefer to see the events feature moving towards a world where THE USER decides which events should be passed to the listener(s). Then the listener(s) would just process every event that is received.
@nandorKollar is working on something in this direction in #3973.
Let's make sure that we eventually make events in Polaris a fully-configurable feature, and not just a feature that satisfies this or that use case.
There was a problem hiding this comment.
I also think we should make event listeners much more configurable. When Polaris starts, it should read the event listener configuration, which defines which listeners should receive which types of events. For example, one could configure a CloudWatch listener to receive only RBAC-related events, while a persistence listener could listen for catalog events.
We could also introduce event categories to make configuration easier. Instead of listing every BEFORE and AFTER catalog event in the configuration, users could simply specify a category such as CATALOG.
#3973 is a first step in this direction: it aims to enable multiple event listeners to watch for events. WDYT?
There was a problem hiding this comment.
I support the idea of letting the user control which event types are processed (persisted).
Are we perhaps at a point where it might be worth splitting the events API proposal from metrics API?
There was a problem hiding this comment.
I'd highly recommend to split to two proposals. These are two different efforts. Mixing together doesn't make sense to me.
| 2. **Phase 2**: Add support for Polaris-specific operations (principals, roles, grants, policies) | ||
| 3. **Phase 3**: Implement the Events REST API on top of the persisted data | ||
|
|
||
| ### 7.2 Database Queries |
There was a problem hiding this comment.
We'd need to evaluate the required changes for the NoSQL backend as well.
There was a problem hiding this comment.
That is worth considering, indeed.
In general, (at least for metrics) I was thinking along the lines of separating scan/commit metrics persistence from MetaStore persistence. This should allow users to use NoSQL metastore and still benefit from JDBC metrics persistence, which might be available before NoSQL Metrics persistence.
The requirements imposed on the metrics Persistence by the query API will certainly affect how NoSQL Metrics persistence ought to be implemented, but I imagine the API should generally not be affected by NoSQL specifics as long as it follows the usual principles for identifying query keys, time range and pagination parameters common to all backends.
There was a problem hiding this comment.
WDYT about using a new PR for this?.. just to allow progress independent of the scan metrics API 🤔
This proposal defines a REST API for querying table scan and commit metrics in Apache Polaris. The API provides endpoints for retrieving metrics reports that have been collected from query engines. Key features: - Single unified /metrics endpoint with type filter parameter - Support for scan and commit metrics types - Pagination support with cursor-based tokens - Authorization aligned with TABLE_READ_DATA privilege Split from combined proposal PR apache#3924 as suggested by reviewer.
The Table Metrics REST API proposal has been moved to a separate PR apache#4010 to allow independent progress on both proposals. This PR now contains only the Events REST API proposal. Requested by: @dimas-b in review comment
obelix74
changed the title
|
Per @dimas-b's suggestion in the review comment, I've split this PR into two separate PRs:
This allows independent progress on both proposals. Thanks for the suggestion! |
7 participants
This proposal defines a REST API for querying catalog events in Apache Polaris, designed to be compatible with the Iceberg Events API specification.
Note: The Table Metrics REST API proposal has been split to a separate PR #4010 to allow independent progress.
Overview
The Events REST API provides endpoints for querying catalog-level audit events, enabling:
Key Design Decisions
x-prefixed operation types for Polaris-specific eventsRelated
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)