flyingImer commented on code in PR #4115: URL: https://github.com/apache/polaris/pull/4115#discussion_r3424090776
########## 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. Review Comment: nit: I think this spec is slightly ahead of what this PR currently implements. It describes a beta API for querying persisted metrics, and the operation text below promises ordered persisted results with filters and pagination. But in this PR, persistence/query backing is intentionally deferred to #4756 and the implementation returns empty pages. ########## extensions/metrics-reports/impl/src/main/java/org/apache/polaris/extension/metrics/reports/LoggingMetricsReporter.java: ########## @@ -16,37 +16,30 @@ * specific language governing permissions and limitations * under the License. */ -package org.apache.polaris.service.reporting; +package org.apache.polaris.extension.metrics.reports; -import com.google.common.annotations.VisibleForTesting; import io.smallrye.common.annotation.Identifier; import jakarta.enterprise.context.ApplicationScoped; import java.time.Instant; import org.apache.iceberg.catalog.TableIdentifier; import org.apache.iceberg.metrics.MetricsReport; +import org.apache.polaris.core.metrics.IcebergMetricsReporter; import org.slf4j.Logger; import org.slf4j.LoggerFactory; /** - * Default implementation of {@link PolarisMetricsReporter} that logs metrics to the configured - * logger. + * Log-only implementation of {@link IcebergMetricsReporter}. * - * <p>This implementation is selected when {@code polaris.iceberg-metrics.reporting.type} is set to - * {@code "default"} (the default value). + * <p>Selected when {@code polaris.iceberg-metrics.reporting.type} is set to {@code "log"}. * - * <p>By default, logging is disabled. To enable metrics logging, set the logger level for {@code - * org.apache.polaris.service.reporting} to {@code INFO} in your logging configuration. - * - * @see PolarisMetricsReporter + * <p>Logging is at INFO level. Enable it by setting the logger level for {@code + * org.apache.polaris.extension.metrics.reports} to {@code INFO}. */ @ApplicationScoped @Identifier("default") Review Comment: nit: the Javadoc says this is selected with `polaris.iceberg-metrics.reporting.type=log`, but the bean is annotated as `@Identifier("default")` below. Since the configured default is now `log`, I think this should probably either be `@Identifier("log")`, or the docs/config examples should consistently call this `default`. My preference would be `log`, since that makes the no-op/log/default vocabulary clearer. ########## extensions/metrics-reports/spi/src/main/java/org/apache/polaris/core/metrics/NoOpMetricsReporter.java: ########## @@ -0,0 +1,44 @@ +/* + * 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. + */ +package org.apache.polaris.core.metrics; + +import io.smallrye.common.annotation.Identifier; +import jakarta.enterprise.context.ApplicationScoped; +import java.time.Instant; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.metrics.MetricsReport; + +/** + * No-op implementation of {@link IcebergMetricsReporter} that silently discards all metrics. + * + * <p>Selected when {@code polaris.iceberg-metrics.reporting.type} is set to {@code "no-op"}. + */ +@ApplicationScoped +@Identifier("no-op") +public class NoOpMetricsReporter implements IcebergMetricsReporter { Review Comment: Minor layering question: this is an implementation, but it lives in the `spi` module and pulls CDI annotations into the SPI artifact. It should go hand in hand along with LoggingMetricsReporter. Also, I'm a bit confused here, why we are having both no op and logging impls? which one is the default? thought this pr is meant to include default impl only? tho I think the other one can sneak in given it is straightforward. IIUC, you are placing no op as the default? if so, lets update the javadoc accordingly as I pointed out above ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/MetricsRecordIdentity.java: ########## @@ -26,105 +26,39 @@ /** * Base interface containing common identification fields shared by all metrics records. * - * <p>This interface defines the common fields that identify the source of a metrics report, - * including the report ID, catalog ID, table ID, timestamp, and metadata. - * * <p>Both {@link ScanMetricsRecord} and {@link CommitMetricsRecord} extend this interface to * inherit these common fields while adding their own specific metrics. * - * <h3>Design Decisions</h3> - * - * <p><b>Entity IDs only (no names):</b> We store only catalog ID and table ID, not their names or - * namespace paths. Names can change over time (via rename operations), which would make querying - * historical metrics by name challenging and lead to correctness issues. Queries should resolve - * names to IDs using the current catalog state. The table ID uniquely identifies the table, and the - * namespace can be derived from the table entity if needed. - * - * <p><b>Realm ID:</b> Realm ID is intentionally not included in this interface. Multi-tenancy realm - * context should be obtained from the CDI-injected {@code RealmContext} at persistence time. This - * keeps catalog-specific code from needing to manage realm concerns. - * * <p><b>Note:</b> This type is part of the experimental Metrics Persistence SPI and may change in * future releases. */ @Beta public interface MetricsRecordIdentity { Review Comment: These fields look more like durable storage/query DTO concerns than the stable reporting/emitting SPI itself: `reportId`, internal IDs, timestamp, request/trace correlation, etc. If durable JDBC metrics is now an optional extension implementation, should these record DTOs move with that extension instead of staying in `polaris-core`? My mental model is: core defines the ingestion/reporting shape; the durable implementation owns its storage records. ########## extensions/metrics-reports/impl/src/main/java/org/apache/polaris/extension/metrics/reports/MetricsReportsService.java: ########## @@ -0,0 +1,154 @@ +/* + * 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. + */ +package org.apache.polaris.extension.metrics.reports; + +import jakarta.annotation.Nonnull; +import jakarta.enterprise.context.RequestScoped; +import jakarta.inject.Inject; +import jakarta.ws.rs.core.Response; +import jakarta.ws.rs.core.SecurityContext; +import java.util.Arrays; +import java.util.List; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.exceptions.NotFoundException; +import org.apache.polaris.core.auth.PolarisAuthorizableOperation; +import org.apache.polaris.core.auth.PolarisAuthorizer; +import org.apache.polaris.core.auth.PolarisPrincipal; +import org.apache.polaris.core.catalog.PolarisCatalogHelpers; +import org.apache.polaris.core.context.CallContext; +import org.apache.polaris.core.context.RealmContext; +import org.apache.polaris.core.entity.PolarisEntitySubType; +import org.apache.polaris.core.entity.PolarisEntityType; +import org.apache.polaris.core.persistence.PolarisMetaStoreManager; +import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper; +import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest; +import org.apache.polaris.core.persistence.resolver.ResolutionManifestFactory; +import org.apache.polaris.core.persistence.resolver.ResolvedPathKey; +import org.apache.polaris.core.persistence.resolver.ResolverPath; +import org.apache.polaris.core.persistence.resolver.ResolverStatus; +import org.apache.polaris.core.rest.NamespaceUtils; +import org.apache.polaris.service.metrics.api.PolarisCatalogsApiService; + +/** + * Service implementation for the Metrics Reports API. + * + * <p>Resolves catalog/namespace/table names to internal IDs and performs authorization. The read + * path currently returns empty results; durable query backing will be provided by the {@code + * extensions/metrics-reports/persistence/relational-jdbc} module in a follow-up. + * + * <p>TODO: wire durable metrics query in the follow-up durable extension PR. + */ +@RequestScoped +public class MetricsReportsService implements PolarisCatalogsApiService { Review Comment: I think this still feels slightly off-layer to me. Not because this should be in the SPI (I totally agree it should not) but because `MetricsReportsService` is a REST/service adapter rather than a metrics extension implementation IMO: It owns the generated API service, HTTP response semantics, namespace decoding, table resolution, and authz. Those feel like `runtime/service` concerns. The metrics extension should own what happens behind the configured metrics implementation, not the public API route shape. Could we keep this service in `runtime/service` and have it delegate to a small query-provider SPI/extension point? Then, if no durable query provider is configured, the route can return an explicit unavailable/not-configured response instead of a successful empty page. The default metrics extension can stay focused on no-op/log behavior, and #4756 can provide the durable JDBC query provider. ########## extensions/metrics-reports/impl/src/main/java/org/apache/polaris/extension/metrics/reports/MetricsReportsService.java: ########## @@ -0,0 +1,154 @@ +/* + * 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. + */ +package org.apache.polaris.extension.metrics.reports; + +import jakarta.annotation.Nonnull; +import jakarta.enterprise.context.RequestScoped; +import jakarta.inject.Inject; +import jakarta.ws.rs.core.Response; +import jakarta.ws.rs.core.SecurityContext; +import java.util.Arrays; +import java.util.List; +import org.apache.iceberg.catalog.Namespace; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.exceptions.NotFoundException; +import org.apache.polaris.core.auth.PolarisAuthorizableOperation; +import org.apache.polaris.core.auth.PolarisAuthorizer; +import org.apache.polaris.core.auth.PolarisPrincipal; +import org.apache.polaris.core.catalog.PolarisCatalogHelpers; +import org.apache.polaris.core.context.CallContext; +import org.apache.polaris.core.context.RealmContext; +import org.apache.polaris.core.entity.PolarisEntitySubType; +import org.apache.polaris.core.entity.PolarisEntityType; +import org.apache.polaris.core.persistence.PolarisMetaStoreManager; +import org.apache.polaris.core.persistence.PolarisResolvedPathWrapper; +import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest; +import org.apache.polaris.core.persistence.resolver.ResolutionManifestFactory; +import org.apache.polaris.core.persistence.resolver.ResolvedPathKey; +import org.apache.polaris.core.persistence.resolver.ResolverPath; +import org.apache.polaris.core.persistence.resolver.ResolverStatus; +import org.apache.polaris.core.rest.NamespaceUtils; +import org.apache.polaris.service.metrics.api.PolarisCatalogsApiService; + +/** + * Service implementation for the Metrics Reports API. + * + * <p>Resolves catalog/namespace/table names to internal IDs and performs authorization. The read + * path currently returns empty results; durable query backing will be provided by the {@code + * extensions/metrics-reports/persistence/relational-jdbc} module in a follow-up. + * + * <p>TODO: wire durable metrics query in the follow-up durable extension PR. + */ +@RequestScoped +public class MetricsReportsService implements PolarisCatalogsApiService { + + private final CallContext callContext; + private final PolarisMetaStoreManager metaStoreManager; + private final PolarisAuthorizer authorizer; + private final PolarisPrincipal polarisPrincipal; + private final ResolutionManifestFactory resolutionManifestFactory; + + @Inject + public MetricsReportsService( + @Nonnull CallContext callContext, + @Nonnull PolarisMetaStoreManager metaStoreManager, + @Nonnull PolarisAuthorizer authorizer, + @Nonnull PolarisPrincipal polarisPrincipal, + @Nonnull ResolutionManifestFactory resolutionManifestFactory) { + this.callContext = callContext; + this.metaStoreManager = metaStoreManager; + this.authorizer = authorizer; + this.polarisPrincipal = polarisPrincipal; + this.resolutionManifestFactory = resolutionManifestFactory; + } + + @Override + public Response listTableMetrics( + String catalogName, + String namespace, + String table, + String metricType, + String pageToken, + Integer pageSize, + Long snapshotId, + String principalName, + Long timestampFrom, + Long timestampTo, + RealmContext realmContext, + SecurityContext securityContext) { + + Namespace ns = decodeNamespace(namespace); + TableIdentifier identifier = TableIdentifier.of(ns, table); + + resolveAndAuthorizeTableMetrics(catalogName, identifier); + + return switch (metricType) { + case "scan" -> Response.ok(new MetricsListResponse<>("scan", null, List.of())).build(); + case "commit" -> Response.ok(new MetricsListResponse<>("commit", null, List.of())).build(); + default -> + throw new IllegalArgumentException( + "Invalid metricType: " + metricType + "; must be 'scan' or 'commit'"); + }; Review Comment: Related to the placement comment above: these branches currently return successful empty pages for both metric types. I worry this makes it hard for a client/operator to distinguish "there are no metrics" from "this server has no durable metrics query implementation configured." Could this route instead return an explicit unavailable/not-configured response until a query provider exists? If the service moves to `runtime/service`, this could be simplified and become the default behavior of returning empty list when no metrics query-provider extension is installed/configured. ########## site/content/in-dev/unreleased/telemetry.md: ########## @@ -191,6 +191,72 @@ polaris.log.mdc.region=us-west-2 MDC context is propagated across threads, including in `TaskExecutor` threads. +## Iceberg Metrics Reports API + +Polaris can persist Iceberg scan and commit metrics reports submitted by clients to the database. Review Comment: nit: This looks stale relative to the current Scope 1 split. The docs say Polaris can persist metrics and then tell operators to set `polaris.iceberg-metrics.reporting.type=persisting`, but this PR removes `PersistingMetricsReporter` and I don't think there is a `@Identifier("persisting")` implementation left here. ########## 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 } + + /** Review Comment: not sure where to put this: this is much less coupled than before, but I think this still leaves one version of the old boundary issue. The Javadoc still frames durable metrics storage as a generic core persistence SPI for "JDBC, NoSQL, custom" backends. Since #4756 is now the durable metrics extension scope, my read is that this contract should probably move there or be deferred to that PR. That would keep `polaris-core` focused on the reporting/emitting boundary, and let `extensions/metrics-reports/persistence/relational-jdbc` own the durable persistence/query contract it actually implements. -- 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]
