flyingImer commented on code in PR #4115: URL: https://github.com/apache/polaris/pull/4115#discussion_r3462102845
########## extensions/metrics-reports/base/src/main/java/org/apache/polaris/extension/metrics/reports/MetricsReportsService.java: ########## @@ -0,0 +1,142 @@ +/* + * 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.enterprise.context.RequestScoped; +import jakarta.inject.Inject; +import jakarta.ws.rs.core.Response; +import jakarta.ws.rs.core.SecurityContext; +import java.util.Arrays; +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.RealmContext; +import org.apache.polaris.core.entity.PolarisEntitySubType; +import org.apache.polaris.core.entity.PolarisEntityType; +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; +import org.jspecify.annotations.NonNull; + +/** + * Service implementation for the Metrics Reports API. + * + * <p>Resolves catalog/namespace/table names to internal IDs and performs authorization. In this + * release the read path returns HTTP 501 Not Implemented; durable query backing is provided by the + * {@code polaris-extensions-metrics-reports-jdbc} extension in a follow-up release. + * + * <p>TODO (#4756): wire durable metrics query provider when the extension is installed. + */ +@RequestScoped +public class MetricsReportsService implements PolarisCatalogsApiService { Review Comment: I would move this out of this PR. This service owns the public REST query route: response codes, namespace decoding, table resolution, and authz. That feels like `runtime/service`, not a metrics extension impl. More importantly, I think the whole query route should land with the durable query provider, not in this SPI/default-impl PR. ########## extensions/metrics-reports/base/src/main/java/org/apache/polaris/extension/metrics/reports/MetricsReportsService.java: ########## @@ -0,0 +1,142 @@ +/* + * 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.enterprise.context.RequestScoped; +import jakarta.inject.Inject; +import jakarta.ws.rs.core.Response; +import jakarta.ws.rs.core.SecurityContext; +import java.util.Arrays; +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.RealmContext; +import org.apache.polaris.core.entity.PolarisEntitySubType; +import org.apache.polaris.core.entity.PolarisEntityType; +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; +import org.jspecify.annotations.NonNull; + +/** + * Service implementation for the Metrics Reports API. + * + * <p>Resolves catalog/namespace/table names to internal IDs and performs authorization. In this + * release the read path returns HTTP 501 Not Implemented; durable query backing is provided by the + * {@code polaris-extensions-metrics-reports-jdbc} extension in a follow-up release. + * + * <p>TODO (#4756): wire durable metrics query provider when the extension is installed. + */ +@RequestScoped +public class MetricsReportsService implements PolarisCatalogsApiService { + + private final PolarisAuthorizer authorizer; + private final PolarisPrincipal polarisPrincipal; + private final ResolutionManifestFactory resolutionManifestFactory; + + @Inject + public MetricsReportsService( + @NonNull PolarisAuthorizer authorizer, + @NonNull PolarisPrincipal polarisPrincipal, + @NonNull ResolutionManifestFactory resolutionManifestFactory) { + 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 Response.status(Response.Status.NOT_IMPLEMENTED) Review Comment: 501 is better than returning empty results, but I still think this shows the query API is not ready for this PR. Could we move the query endpoint to the durable metrics PR, so the route and backing implementation land together? ########## 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 look like durable storage records, not reporting SPI types. I would move them with the durable extension instead of keeping them in core. ########## runtime/service/src/main/java/org/apache/polaris/service/reporting/MetricsReportingConfiguration.java: ########## @@ -23,6 +23,6 @@ @ConfigMapping(prefix = "polaris.iceberg-metrics.reporting") public interface MetricsReportingConfiguration { - @WithDefault("default") + @WithDefault("log") Review Comment: I think the default should be `no-op`. Log is useful, but scan metrics can be high-volume. I would make log opt-in and keep the built-in default silent/safe. ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/CommitMetricsRecord.java: ########## @@ -25,101 +25,55 @@ /** * Backend-agnostic representation of an Iceberg commit metrics report. * - * <p>This record captures all relevant metrics from an Iceberg {@code CommitReport} along with - * contextual information such as catalog identification and table location. - * - * <p>Common identification fields are inherited from {@link MetricsRecordIdentity}. - * - * <p>Note: Realm ID is not included in this record. Multi-tenancy realm context should be obtained - * from the CDI-injected {@code RealmContext} at persistence time. - * * <p><b>Note:</b> This type is part of the experimental Metrics Persistence SPI and may change in * future releases. */ @Beta @PolarisImmutable public interface CommitMetricsRecord extends MetricsRecordIdentity { Review Comment: I would move/defer this to the durable metrics PR. This is persistence-specific, and durable metrics is now extension work. I do not think core should define a JDBC/NoSQL/custom metrics persistence SPI in this PR. And if we introduce them in the following prs, they should be under extensions/ to be self-contained along with the whole durable metrics spi impl ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/PolarisMetricsManager.java: ########## @@ -34,11 +34,6 @@ * <p>Request context (principal name, request ID, OTEL trace/span IDs) should be populated in the * record by the caller before invoking these methods. This keeps the SPI simple with a single * method parameter containing all the data needed for persistence. - * - * <p>Since {@link org.apache.polaris.core.persistence.BasePersistence} now extends {@link - * MetricsPersistence} with default no-op implementations, all persistence backends automatically - * support this interface. Backends that want actual metrics persistence (e.g., JDBC) override the - * methods; others use the default no-op behavior. */ public interface PolarisMetricsManager { Review Comment: Same concern: this keeps metrics persistence on the metastore-manager path. For this PR, I would prefer only the reporting SPI. The persistence manager can move with the durable implementation if it is still needed there. -- 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]
