flyingImer commented on code in PR #4115:
URL: https://github.com/apache/polaris/pull/4115#discussion_r3400252874
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/MetricsPersistence.java:
##########
@@ -73,4 +77,56 @@ default void writeScanReport(@NonNull ScanMetricsRecord
record) {
default void writeCommitReport(@NonNull CommitMetricsRecord record) {
// No-op by default - backends that don't support metrics silently ignore
}
+
+ /**
+ * Lists scan metrics reports for a given table, ordered by timestamp
descending.
+ *
+ * <p>Default implementation returns an empty page. Override in
implementations that support
+ * metrics reads.
+ *
+ * @param catalogId the internal catalog ID
+ * @param tableId the internal table entity ID
+ * @param snapshotId optional filter by snapshot ID
+ * @param principalName optional filter by principal name
+ * @param timestampFrom optional inclusive lower bound on timestamp (epoch
ms)
+ * @param timestampTo optional exclusive upper bound on timestamp (epoch ms)
+ * @param pageToken pagination token
+ * @return a page of scan metrics records
+ */
+ default Page<ScanMetricsRecord> listScanReports(
Review Comment:
Per the latest [metrics architecture sync
notes](https://docs.google.com/document/d/100h7c4damrUzVuquYbBHM0EvA4LSWuW2IT2dN_7nYVA/edit?pli=1&tab=t.k96s2xyqr5u1#heading=h.uvb454otvxc0),
I think this is the main boundary we should avoid standardizing in this PR.
The alignment there was that the stable SPI should be Iceberg metrics
reporting/emitting, not metrics persistence inherited by every
`BasePersistence` implementation (which has already been covered by #4397).
Durable metrics storage can still exist, but as a durable metrics reporter
implementation detail under extensions/ module.
Could we avoid introducing this as the SPI in `polaris-core` and instead
move the contract toward an `IcebergMetricsReporter` + context boundary in the
metrics extension API layer?
##########
spec/metrics-reports-service.yml:
##########
@@ -0,0 +1,483 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+#
+
+openapi: 3.0.3
+info:
+ title: Apache Polaris Metrics Reports API
+ description: >
+ **Beta**: Read-only API for querying persisted Iceberg table metrics (scan
and commit reports)
+ from Apache Polaris. This API is in beta and may change in future releases.
+ Requires TABLE_READ_METRICS privilege on the target table.
+ version: 0.1.0
+ license:
+ name: Apache 2.0
+ url: https://www.apache.org/licenses/LICENSE-2.0.html
+
+servers:
+ - url: "{scheme}://{host}/api/metrics-reports/v1"
+ variables:
+ scheme:
+ default: https
+ host:
+ default: localhost
+
+paths:
+ /catalogs/{catalogName}/namespaces/{namespace}/tables/{table}:
+ parameters:
+ - $ref: '#/components/parameters/catalogName'
+ - $ref: '#/components/parameters/namespace'
+ - $ref: '#/components/parameters/table'
+ get:
+ operationId: listTableMetrics
+ summary: List metrics reports for a table
+ description: >
+ Returns persisted metrics reports for the specified table. The
required `metricType`
+ parameter selects between scan reports (produced during table reads)
and commit reports
+ (produced during table writes). Results are ordered by timestamp
descending.
+ Requires TABLE_READ_METRICS privilege on the target table.
+ tags:
+ - Metrics
+ parameters:
+ - name: metricType
+ in: query
+ required: true
+ description: Type of metrics to retrieve
+ schema:
+ type: string
+ enum: [scan, commit]
+ - name: pageToken
+ in: query
+ required: false
+ schema:
+ type: string
+ description: Opaque cursor from a previous response's
nextPageToken field
+ - name: pageSize
+ in: query
+ required: false
+ schema:
+ type: integer
+ minimum: 1
+ default: 100
+ description: Maximum number of results to return per page
+ - name: snapshotId
+ in: query
+ required: false
+ schema:
+ type: integer
+ format: int64
+ description: Filter results to a specific snapshot ID
+ - name: principalName
+ in: query
+ required: false
+ schema:
+ type: string
+ description: Filter results to a specific principal (e.g. service
account name)
+ - name: timestampFrom
+ in: query
+ required: false
+ schema:
+ type: integer
+ format: int64
+ description: Inclusive lower bound on report timestamp (Unix epoch
milliseconds)
+ - name: timestampTo
+ in: query
+ required: false
+ schema:
+ type: integer
+ format: int64
+ description: Exclusive upper bound on report timestamp (Unix epoch
milliseconds)
+ responses:
+ '200':
+ description: Paginated list of metrics reports
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/ListMetricsResponse'
+ '400':
+ description: Bad request (missing or invalid parameters)
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/ErrorResponse'
+ '403':
+ description: Insufficient privileges (TABLE_READ_METRICS required)
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/ErrorResponse'
+ '404':
+ description: Catalog, namespace, or table not found
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/ErrorResponse'
+
+components:
+ parameters:
+ catalogName:
+ name: catalogName
+ in: path
+ required: true
+ schema:
+ type: string
+ namespace:
+ name: namespace
+ in: path
+ required: true
+ description: >
+ Namespace path encoded using the same convention as the Polaris
Iceberg REST API:
+ multi-level namespaces use the unit separator (0x1F) between levels,
URL-encoded as %1F.
+ schema:
+ type: string
+ table:
+ name: table
+ in: path
+ required: true
+ schema:
+ type: string
+
+ schemas:
+ ListMetricsResponse:
+ description: >
+ Polymorphic response for metrics queries. The concrete type is
determined by the
+ metricType discriminator field, which echoes the requested metricType
query parameter.
+ oneOf:
+ - $ref: '#/components/schemas/ListScanMetricsResponse'
+ - $ref: '#/components/schemas/ListCommitMetricsResponse'
Review Comment:
The beta API shape here is probably okay to keep moving, but I think it
should be clearly decoupled from the durable storage/SPI shape.
In other words, I'm less concerned about this response envelope than about
what backs it. Could we keep the REST API work scoped as beta while moving the
durable persistence backing out of the core metastore persistence path?
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -1347,4 +1348,141 @@ private void writeCommitMetricsReport(@NonNull
ModelCommitMetricsReport report)
String.format("Failed to write commit metrics report due to %s",
e.getMessage()), e);
}
}
+
+ @Override
+ public Page<ScanMetricsRecord> listScanReports(
Review Comment:
This is another sign that the durable implementation is landing in the
generic JDBC metastore backend rather than in a metrics-specific durable
implementation.
I think the long-term shape should let a deployment use one backend for the
metastore and another for metrics. Putting the metrics read/write/query logic
directly in `JdbcBasePersistenceImpl` makes that separation harder. Could this
move under the durable metrics implementation module instead, alongside its
schema/bootstrap/read API support?
##########
extensions/metrics-reports/impl/build.gradle.kts:
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+plugins {
+ id("polaris-server")
+ id("org.kordamp.gradle.jandex")
+}
+
+dependencies {
+ implementation(project(":polaris-core"))
+ implementation(project(":polaris-api-metrics-reports-service"))
Review Comment:
Naming/scope question: this module is called an extension, but the durable
implementation pieces are still spread across `polaris-core`,
`runtime/service`, and `persistence/relational-jdbc`.
If this is meant to be the metrics extension, could we make it own the
durable metrics implementation more fully? Otherwise the module name may imply
a cleaner extension boundary than the code currently has.
##########
gradle/projects.main.properties:
##########
@@ -49,6 +50,7 @@ polaris-extensions-federation-hive=extensions/federation/hive
polaris-extensions-federation-bigquery=extensions/federation/bigquery
polaris-extensions-auth-opa=extensions/auth/opa/impl
polaris-extensions-auth-opa-tests=extensions/auth/opa/tests
+polaris-extensions-metrics-reports=extensions/metrics-reports/impl
Review Comment:
If we split the scope, I think the module structure could make the intended
boundary more obvious:
- an API/SPI layer for Iceberg metrics reporting + default no-op/log-only
battery
- a durable JDBC metrics implementation module that owns its
persistence/schema/read support
That would avoid the generic `metrics-reports` extension name becoming a
catch-all for both API surface and durable storage concerns.
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -1347,4 +1348,141 @@ private void writeCommitMetricsReport(@NonNull
ModelCommitMetricsReport report)
String.format("Failed to write commit metrics report due to %s",
e.getMessage()), e);
}
}
+
+ @Override
+ public Page<ScanMetricsRecord> listScanReports(
Review Comment:
Also, for the read path specifically, this bakes the per-type query API into
the low-level persistence surface.
Given the sync alignment around treating the current scan/commit split as
transitional, I'd prefer not to add `listScanReports`/`listCommitReports` as
core persistence methods. A typed query behind the durable metrics
implementation would leave us more room to consolidate the schema/model without
another SPI churn.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
