dimas-b commented on code in PR #3616: URL: https://github.com/apache/polaris/pull/3616#discussion_r2761627262
########## polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/ScanMetricsRecord.java: ########## @@ -0,0 +1,162 @@ +/* + * 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.persistence.metrics; + +import java.time.Instant; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import org.apache.polaris.immutables.PolarisImmutable; + +/** + * Backend-agnostic representation of an Iceberg scan metrics report. + * + * <p>This record captures all relevant metrics from an Iceberg {@code ScanReport} along with + * contextual information such as realm, catalog, and request correlation data. + */ +@PolarisImmutable +public interface ScanMetricsRecord { + + // === Identification === + + /** Unique identifier for this report (UUID). */ + String reportId(); + + /** Multi-tenancy realm identifier. */ + String realmId(); + + /** Internal catalog ID. */ + String catalogId(); + + /** Human-readable catalog name. */ + String catalogName(); + + /** Dot-separated namespace path (e.g., "db.schema"). */ + String namespace(); + + /** Table name. */ + String tableName(); + + // === Timing === + + /** Timestamp when the report was received. */ + Instant timestamp(); + + // === Client Correlation === + + /** + * Client-provided trace ID from the metrics report metadata. + * + * <p>This is an optional identifier that the Iceberg client may include in the report's metadata + * map (typically under the key "trace-id"). It allows clients to correlate this metrics report + * with their own distributed tracing system or query execution context. + * + * <p>Note: Server-side tracing information (e.g., OpenTelemetry trace/span IDs) and principal + * information are not included in this record. The persistence implementation can obtain these + * from the ambient request context (OTel context, security context) at write time if needed. + */ + Optional<String> reportTraceId(); Review Comment: I'd prefer to track it as a contextual piece of data like OTel trace info... WDYT? ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/ScanMetricsRecord.java: ########## @@ -0,0 +1,162 @@ +/* + * 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.persistence.metrics; + +import java.time.Instant; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import org.apache.polaris.immutables.PolarisImmutable; + +/** + * Backend-agnostic representation of an Iceberg scan metrics report. + * + * <p>This record captures all relevant metrics from an Iceberg {@code ScanReport} along with + * contextual information such as realm, catalog, and request correlation data. + */ +@PolarisImmutable +public interface ScanMetricsRecord { + + // === Identification === + + /** Unique identifier for this report (UUID). */ + String reportId(); + + /** Multi-tenancy realm identifier. */ + String realmId(); Review Comment: Realm ID is generally part of CDI context. I believe it is preferable to keep it out of catalog-specific classes so that the code working inside a catalog won't have to worry about the realm. Of course, persistence impl. will have to take realm into account. ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/ScanMetricsRecord.java: ########## @@ -0,0 +1,162 @@ +/* + * 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.persistence.metrics; + +import java.time.Instant; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import org.apache.polaris.immutables.PolarisImmutable; + +/** + * Backend-agnostic representation of an Iceberg scan metrics report. + * + * <p>This record captures all relevant metrics from an Iceberg {@code ScanReport} along with + * contextual information such as realm, catalog, and request correlation data. + */ +@PolarisImmutable +public interface ScanMetricsRecord { + + // === Identification === + + /** Unique identifier for this report (UUID). */ + String reportId(); + + /** Multi-tenancy realm identifier. */ + String realmId(); + + /** Internal catalog ID. */ + String catalogId(); Review Comment: Why `String`? `PolarisEntityCore` defines it as a `long` 🤔 ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/CommitMetricsRecord.java: ########## @@ -0,0 +1,163 @@ +/* + * 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.persistence.metrics; + +import java.time.Instant; +import java.util.Map; +import java.util.Optional; +import org.apache.polaris.immutables.PolarisImmutable; + +/** + * 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 realm, catalog, and request correlation data. + */ +@PolarisImmutable +public interface CommitMetricsRecord { + + // === Identification === + + /** Unique identifier for this report (UUID). */ + String reportId(); + + /** Multi-tenancy realm identifier. */ + String realmId(); + + /** Internal catalog ID. */ + String catalogId(); + + /** Human-readable catalog name. */ + String catalogName(); Review Comment: Would it be possible to move common data about the entity associated with the report (scan / commit) into a share (base?) interface? ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/MetricsRecordConverter.java: ########## @@ -0,0 +1,180 @@ +/* + * 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.persistence.metrics; + +import java.time.Instant; +import java.util.Collections; +import java.util.Optional; +import java.util.UUID; +import org.apache.iceberg.metrics.CommitMetricsResult; +import org.apache.iceberg.metrics.CommitReport; +import org.apache.iceberg.metrics.CounterResult; +import org.apache.iceberg.metrics.ScanMetricsResult; +import org.apache.iceberg.metrics.ScanReport; +import org.apache.iceberg.metrics.TimerResult; + +/** + * Utility class for converting Iceberg metrics reports to SPI record types. + * + * <p>This converter extracts all relevant metrics from Iceberg's {@link ScanReport} and {@link + * CommitReport} and combines them with context information to create persistence-ready records. + */ +public final class MetricsRecordConverter { + + private MetricsRecordConverter() { + // Utility class + } + + /** + * Converts an Iceberg ScanReport to a ScanMetricsRecord. + * + * @param scanReport the Iceberg scan report + * @param tableName the table name + * @param context the metrics context containing realm, catalog, and request information + * @return the scan metrics record ready for persistence + */ + public static ScanMetricsRecord fromScanReport( + ScanReport scanReport, String tableName, MetricsContext context) { Review Comment: `MetricsContext` feels like a group of parameters for convenience... WDYT about this pattern: `MetricsRecordConverter.forTable(catalogName, tableName, etc...).convert(ScanReport scanReport)`? ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/ReportIdToken.java: ########## @@ -0,0 +1,122 @@ +/* + * 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.persistence.metrics; + +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.databind.annotation.JsonDeserialize; +import com.fasterxml.jackson.databind.annotation.JsonSerialize; +import jakarta.annotation.Nullable; +import org.apache.polaris.core.persistence.pagination.Token; +import org.apache.polaris.immutables.PolarisImmutable; + +/** + * Pagination {@linkplain Token token} for metrics queries, backed by the report ID (UUID). + * + * <p>This token enables cursor-based pagination for metrics queries across different storage + * backends. The report ID is used as the cursor because it is: + * + * <ul> + * <li>Guaranteed unique across all reports + * <li>Present in both scan and commit metrics records + * <li>Stable (doesn't change over time) + * </ul> + * + * <p>Each backend implementation can use this cursor value to implement efficient pagination in + * whatever way is optimal for that storage system: + * + * <ul> + * <li>RDBMS: {@code WHERE report_id > :lastReportId ORDER BY report_id} + * <li>NoSQL: Use report ID as partition/sort key cursor + * <li>Time-series: Combine with timestamp for efficient range scans Review Comment: nit: This can probably work, but I believe it is preferable to allow the Persistence impl. to choose a token impl. that fits the backend's features. It is ok to keep this class if you're planning to use it for JDBC... I just want to make sure that this token is not guaranteed to be used by all backends. ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/ScanMetricsRecord.java: ########## @@ -0,0 +1,162 @@ +/* + * 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.persistence.metrics; + +import java.time.Instant; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import org.apache.polaris.immutables.PolarisImmutable; + +/** + * Backend-agnostic representation of an Iceberg scan metrics report. + * + * <p>This record captures all relevant metrics from an Iceberg {@code ScanReport} along with + * contextual information such as realm, catalog, and request correlation data. + */ +@PolarisImmutable +public interface ScanMetricsRecord { + + // === Identification === + + /** Unique identifier for this report (UUID). */ + String reportId(); + + /** Multi-tenancy realm identifier. */ + String realmId(); + + /** Internal catalog ID. */ + String catalogId(); + + /** Human-readable catalog name. */ + String catalogName(); Review Comment: WDYT about using `CatalogEntity` instead of separate name / ID attributes? ... same for namespace / table. ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/MetricsPersistence.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.core.persistence.metrics; + +import jakarta.annotation.Nonnull; +import org.apache.polaris.core.persistence.pagination.Page; +import org.apache.polaris.core.persistence.pagination.PageToken; + +/** + * Service Provider Interface (SPI) for persisting Iceberg metrics reports. + * + * <p>This interface enables different persistence backends (JDBC, NoSQL, custom) to implement + * metrics storage in a way appropriate for their storage model, while allowing service code to + * remain backend-agnostic. + * + * <p>Implementations should be idempotent - writing the same reportId twice should have no effect. + * Implementations that don't support metrics persistence should return {@link #NOOP}. + * + * <h3>Pagination</h3> + * + * <p>Query methods use the standard Polaris pagination pattern with {@link PageToken} for requests + * and {@link Page} for responses. This enables: + * + * <ul> + * <li>Backend-specific cursor implementations (RDBMS offset, NoSQL continuation tokens, etc.) + * <li>Consistent pagination interface across all Polaris persistence APIs + * <li>Efficient cursor-based pagination that works with large result sets + * </ul> + * + * <p>The {@link ReportIdToken} provides a cursor based on the report ID (UUID), but backends may + * use other cursor strategies internally. + * + * @see PageToken + * @see Page + * @see ReportIdToken + */ +public interface MetricsPersistence { + + /** A no-op implementation for backends that don't support metrics persistence. */ + MetricsPersistence NOOP = new NoOpMetricsPersistence(); + + // ============================================================================ + // Capability Detection + // ============================================================================ + + /** + * Returns whether this persistence backend supports metrics storage. + * + * <p>Backends that do not support metrics should return false. Service code should NOT use this + * to branch with instanceof checks - instead, call the interface methods directly and rely on the + * no-op behavior for unsupported backends. + * + * @return true if metrics persistence is supported, false otherwise + */ + boolean isSupported(); Review Comment: Is this envisioned to be used as an optimization of some kind? Why not just call methods, which will not do anything (when not supported)? ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/MetricsPersistenceFactory.java: ########## @@ -0,0 +1,41 @@ +/* + * 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.persistence.metrics; + +import org.apache.polaris.core.context.RealmContext; + +/** + * Factory interface for creating {@link MetricsPersistence} instances. + * + * <p>Implementations may cache instances per realm for efficiency. For backends that don't support + * metrics persistence, implementations should return {@link MetricsPersistence#NOOP}. + */ +public interface MetricsPersistenceFactory { + + /** + * Gets or creates a {@link MetricsPersistence} instance for the given realm. + * + * <p>Implementations may cache instances per realm. If the persistence backend does not support + * metrics persistence, this method should return {@link MetricsPersistence#NOOP}. + * + * @param realmContext the realm context + * @return a MetricsPersistence instance for the realm, or NOOP if not supported + */ + MetricsPersistence getOrCreateMetricsPersistence(RealmContext realmContext); Review Comment: Is a factory really required? Would `ServiceProducers` be able to produce an injectable bean without a factory? In any case, the factory feel more like a deployment / CDI concern than a core SPI concern. WDYT? ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/MetricsRecordConverter.java: ########## @@ -0,0 +1,180 @@ +/* + * 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.persistence.metrics; + +import java.time.Instant; +import java.util.Collections; +import java.util.Optional; +import java.util.UUID; +import org.apache.iceberg.metrics.CommitMetricsResult; +import org.apache.iceberg.metrics.CommitReport; +import org.apache.iceberg.metrics.CounterResult; +import org.apache.iceberg.metrics.ScanMetricsResult; +import org.apache.iceberg.metrics.ScanReport; +import org.apache.iceberg.metrics.TimerResult; + +/** + * Utility class for converting Iceberg metrics reports to SPI record types. + * + * <p>This converter extracts all relevant metrics from Iceberg's {@link ScanReport} and {@link + * CommitReport} and combines them with context information to create persistence-ready records. + */ +public final class MetricsRecordConverter { Review Comment: This converter class makes sense, but I guess it is technically not part of the "persistence" SPI... Do you think we could keep it in a different package? Then this SPI will be detached from Iceberg REST API details. ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/MetricsPersistence.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.core.persistence.metrics; + +import jakarta.annotation.Nonnull; +import org.apache.polaris.core.persistence.pagination.Page; +import org.apache.polaris.core.persistence.pagination.PageToken; + +/** + * Service Provider Interface (SPI) for persisting Iceberg metrics reports. + * + * <p>This interface enables different persistence backends (JDBC, NoSQL, custom) to implement + * metrics storage in a way appropriate for their storage model, while allowing service code to + * remain backend-agnostic. + * + * <p>Implementations should be idempotent - writing the same reportId twice should have no effect. + * Implementations that don't support metrics persistence should return {@link #NOOP}. + * + * <h3>Pagination</h3> + * + * <p>Query methods use the standard Polaris pagination pattern with {@link PageToken} for requests + * and {@link Page} for responses. This enables: + * + * <ul> + * <li>Backend-specific cursor implementations (RDBMS offset, NoSQL continuation tokens, etc.) + * <li>Consistent pagination interface across all Polaris persistence APIs + * <li>Efficient cursor-based pagination that works with large result sets + * </ul> + * + * <p>The {@link ReportIdToken} provides a cursor based on the report ID (UUID), but backends may + * use other cursor strategies internally. + * + * @see PageToken + * @see Page + * @see ReportIdToken + */ +public interface MetricsPersistence { + + /** A no-op implementation for backends that don't support metrics persistence. */ + MetricsPersistence NOOP = new NoOpMetricsPersistence(); + + // ============================================================================ + // Capability Detection + // ============================================================================ + + /** + * Returns whether this persistence backend supports metrics storage. + * + * <p>Backends that do not support metrics should return false. Service code should NOT use this + * to branch with instanceof checks - instead, call the interface methods directly and rely on the + * no-op behavior for unsupported backends. + * + * @return true if metrics persistence is supported, false otherwise + */ + boolean isSupported(); + + // ============================================================================ + // Write Operations + // ============================================================================ + + /** + * Persists a scan metrics record. + * + * <p>This operation is idempotent - writing the same reportId twice has no effect. If {@link + * #isSupported()} returns false, this is a no-op. + * + * @param record the scan metrics record to persist + */ + void writeScanReport(@Nonnull ScanMetricsRecord record); + + /** + * Persists a commit metrics record. + * + * <p>This operation is idempotent - writing the same reportId twice has no effect. If {@link + * #isSupported()} returns false, this is a no-op. + * + * @param record the commit metrics record to persist + */ + void writeCommitReport(@Nonnull CommitMetricsRecord record); + + // ============================================================================ + // Query Operations + // ============================================================================ + + /** + * Queries scan metrics reports based on the specified criteria. + * + * <p>Returns an empty page if {@link #isSupported()} returns false. + * + * <p>Example usage: + * + * <pre>{@code + * // First page + * PageToken pageToken = PageToken.fromLimit(100); + * Page<ScanMetricsRecord> page = persistence.queryScanReports(criteria, pageToken); + * + * // Next page (if available) + * String nextPageToken = page.encodedResponseToken(); + * if (nextPageToken != null) { + * pageToken = PageToken.build(nextPageToken, null, () -> true); + * Page<ScanMetricsRecord> nextPage = persistence.queryScanReports(criteria, pageToken); + * } + * }</pre> + * + * @param criteria the query criteria (filters) + * @param pageToken pagination parameters (page size and optional cursor) + * @return page of matching scan metrics records with continuation token if more results exist + */ + @Nonnull + Page<ScanMetricsRecord> queryScanReports( + @Nonnull MetricsQueryCriteria criteria, @Nonnull PageToken pageToken); Review Comment: `MetricsQueryCriteria` provides a lot of options, but it is not clear which permutations need to be supported. WDYT about adding a separate query method per query pattern (with individual parameters)? ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/MetricsPersistence.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.core.persistence.metrics; + +import jakarta.annotation.Nonnull; +import org.apache.polaris.core.persistence.pagination.Page; +import org.apache.polaris.core.persistence.pagination.PageToken; + +/** + * Service Provider Interface (SPI) for persisting Iceberg metrics reports. + * + * <p>This interface enables different persistence backends (JDBC, NoSQL, custom) to implement + * metrics storage in a way appropriate for their storage model, while allowing service code to + * remain backend-agnostic. + * + * <p>Implementations should be idempotent - writing the same reportId twice should have no effect. + * Implementations that don't support metrics persistence should return {@link #NOOP}. + * + * <h3>Pagination</h3> + * + * <p>Query methods use the standard Polaris pagination pattern with {@link PageToken} for requests + * and {@link Page} for responses. This enables: + * + * <ul> + * <li>Backend-specific cursor implementations (RDBMS offset, NoSQL continuation tokens, etc.) + * <li>Consistent pagination interface across all Polaris persistence APIs + * <li>Efficient cursor-based pagination that works with large result sets + * </ul> + * + * <p>The {@link ReportIdToken} provides a cursor based on the report ID (UUID), but backends may + * use other cursor strategies internally. + * + * @see PageToken + * @see Page + * @see ReportIdToken + */ +public interface MetricsPersistence { + + /** A no-op implementation for backends that don't support metrics persistence. */ + MetricsPersistence NOOP = new NoOpMetricsPersistence(); + + // ============================================================================ + // Capability Detection + // ============================================================================ + + /** + * Returns whether this persistence backend supports metrics storage. + * + * <p>Backends that do not support metrics should return false. Service code should NOT use this + * to branch with instanceof checks - instead, call the interface methods directly and rely on the + * no-op behavior for unsupported backends. + * + * @return true if metrics persistence is supported, false otherwise + */ + boolean isSupported(); + + // ============================================================================ + // Write Operations + // ============================================================================ + + /** + * Persists a scan metrics record. + * + * <p>This operation is idempotent - writing the same reportId twice has no effect. If {@link + * #isSupported()} returns false, this is a no-op. + * + * @param record the scan metrics record to persist + */ + void writeScanReport(@Nonnull ScanMetricsRecord record); + + /** + * Persists a commit metrics record. + * + * <p>This operation is idempotent - writing the same reportId twice has no effect. If {@link + * #isSupported()} returns false, this is a no-op. + * + * @param record the commit metrics record to persist + */ + void writeCommitReport(@Nonnull CommitMetricsRecord record); + + // ============================================================================ + // Query Operations + // ============================================================================ + + /** + * Queries scan metrics reports based on the specified criteria. + * + * <p>Returns an empty page if {@link #isSupported()} returns false. + * + * <p>Example usage: + * + * <pre>{@code + * // First page + * PageToken pageToken = PageToken.fromLimit(100); + * Page<ScanMetricsRecord> page = persistence.queryScanReports(criteria, pageToken); + * + * // Next page (if available) + * String nextPageToken = page.encodedResponseToken(); + * if (nextPageToken != null) { + * pageToken = PageToken.build(nextPageToken, null, () -> true); + * Page<ScanMetricsRecord> nextPage = persistence.queryScanReports(criteria, pageToken); + * } + * }</pre> + * + * @param criteria the query criteria (filters) + * @param pageToken pagination parameters (page size and optional cursor) + * @return page of matching scan metrics records with continuation token if more results exist + */ + @Nonnull + Page<ScanMetricsRecord> queryScanReports( + @Nonnull MetricsQueryCriteria criteria, @Nonnull PageToken pageToken); Review Comment: If query patterns are the same between scan and commit reports, we could model that as a sub-interface, e.g. `scanReportQuery().execute(..., PageToken)` and `commitReportQuery().execute(..., PageToken)` ########## polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/MetricsQueryCriteria.java: ########## @@ -0,0 +1,153 @@ +/* + * 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.persistence.metrics; + +import java.time.Instant; +import java.util.Optional; +import org.apache.polaris.immutables.PolarisImmutable; + +/** + * Query criteria for retrieving metrics reports. + * + * <p>This class defines the filter parameters for metrics queries. Pagination is handled separately + * via {@link org.apache.polaris.core.persistence.pagination.PageToken}, which is passed as a + * separate parameter to query methods. This separation of concerns allows: + * + * <ul> + * <li>Different backends to implement pagination in their optimal way + * <li>Cursor-based pagination that works with both RDBMS and NoSQL backends + * <li>Reuse of the existing Polaris pagination infrastructure + * </ul> + * + * <h3>Supported Query Patterns</h3> + * + * <table> + * <tr><th>Pattern</th><th>Fields Used</th><th>Index Required</th></tr> + * <tr><td>By Table + Time</td><td>catalogName, namespace, tableName, startTime, endTime</td><td>Yes (OSS)</td></tr> + * <tr><td>By Client Trace ID</td><td>reportTraceId</td><td>No (custom deployment)</td></tr> + * <tr><td>By Time Only</td><td>startTime, endTime</td><td>Partial (timestamp index)</td></tr> + * </table> + * + * <h3>Pagination</h3> + * + * <p>Pagination is handled via the {@link org.apache.polaris.core.persistence.pagination.PageToken} + * passed to query methods. The token contains: + * + * <ul> + * <li>{@code pageSize()} - Maximum number of results to return + * <li>{@code value()} - Optional cursor token (e.g., {@link ReportIdToken}) for continuation + * </ul> + * + * <p>Query results are returned as {@link org.apache.polaris.core.persistence.pagination.Page} + * which includes an encoded token for fetching the next page. + * + * @see org.apache.polaris.core.persistence.pagination.PageToken + * @see org.apache.polaris.core.persistence.pagination.Page + * @see ReportIdToken + */ +@PolarisImmutable +public interface MetricsQueryCriteria { + + // === Table Identification (optional) === + + /** Catalog name to filter by. */ + Optional<String> catalogName(); + + /** Namespace to filter by (dot-separated). */ + Optional<String> namespace(); + + /** Table name to filter by. */ + Optional<String> tableName(); + + // === Time Range === + + /** Start time for the query (inclusive). */ + Optional<Instant> startTime(); + + /** End time for the query (exclusive). */ + Optional<Instant> endTime(); + + // === Correlation === + + /** + * Client-provided trace ID to filter by (from report metadata). + * + * <p>This matches the {@code reportTraceId} field in the metrics records, which originates from + * the client's metadata map. Useful for correlating metrics with client-side query execution. + * + * <p>Note: This query pattern may require a custom index in deployment environments. The OSS + * codebase does not include an index for trace-based queries. + */ + Optional<String> reportTraceId(); Review Comment: I'm not sure trace IDs are so well-defined in Polaris ATM to be used as a primary query parameter 🤔 Are you adding this just because trace IDs were mentioned elsewhere, or do you have a concrete use case in mind? -- 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]
