Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
dimas-b merged PR #3616: URL: https://github.com/apache/polaris/pull/3616 -- 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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
dimas-b commented on PR #3616: URL: https://github.com/apache/polaris/pull/3616#issuecomment-3873265000 @singhpk234 : Are you ok to merge this PR? -- 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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2775590123
##
polaris-core/src/main/java/org/apache/polaris/core/metrics/iceberg/MetricsRecordConverter.java:
##
@@ -0,0 +1,300 @@
+/*
+ * 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.iceberg;
+
+import java.time.Instant;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+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;
+import org.apache.polaris.core.persistence.metrics.CommitMetricsRecord;
+import org.apache.polaris.core.persistence.metrics.ScanMetricsRecord;
+
+/**
+ * Converts Iceberg metrics reports to SPI record types using a fluent builder
API.
+ *
+ * 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.
+ *
+ * Example usage:
+ *
+ * {@code
+ * ScanMetricsRecord record = MetricsRecordConverter.forScanReport(scanReport)
+ * .catalogId(catalog.getId())
+ * .tableId(tableEntity.getId())
+ * .namespace(namespace)
+ * .build();
+ * }
+ */
+public final class MetricsRecordConverter {
+
+ private MetricsRecordConverter() {
+// Utility class
+ }
+
+ /**
+ * Creates a builder for converting a ScanReport to a ScanMetricsRecord.
+ *
+ * @param scanReport the Iceberg scan report
+ * @return builder for configuring the conversion
+ */
+ public static ScanReportBuilder forScanReport(ScanReport scanReport) {
+return new ScanReportBuilder(scanReport);
+ }
+
+ /**
+ * Creates a builder for converting a CommitReport to a CommitMetricsRecord.
+ *
+ * @param commitReport the Iceberg commit report
+ * @return builder for configuring the conversion
+ */
+ public static CommitReportBuilder forCommitReport(CommitReport commitReport)
{
+return new CommitReportBuilder(commitReport);
+ }
+
+ /** Builder for converting ScanReport to ScanMetricsRecord. */
+ public static final class ScanReportBuilder {
+private final ScanReport scanReport;
+private long catalogId;
+private long tableId;
+private List namespace = Collections.emptyList();
+private Instant timestamp;
+
+private ScanReportBuilder(ScanReport scanReport) {
+ this.scanReport = scanReport;
+}
+
+public ScanReportBuilder catalogId(long catalogId) {
+ this.catalogId = catalogId;
+ return this;
+}
+
+/**
+ * Sets the table entity ID.
+ *
+ * This is the internal Polaris entity ID for the table.
+ *
+ * @param tableId the table entity ID
+ * @return this builder
+ */
+public ScanReportBuilder tableId(long tableId) {
+ this.tableId = tableId;
+ return this;
+}
+
+/**
+ * Sets the namespace as a list of levels.
+ *
+ * @param namespace the namespace levels
+ * @return this builder
+ */
+public ScanReportBuilder namespace(List namespace) {
+ this.namespace = namespace != null ? namespace : Collections.emptyList();
+ return this;
+}
Review Comment:
As discussed, removed namespace.
• Removed namespace() method from MetricsRecordIdentity.java
• Removed namespace() calls from MetricsRecordConverter.java builders
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
singhpk234 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2772723554
##
polaris-core/src/main/java/org/apache/polaris/core/metrics/iceberg/MetricsRecordConverter.java:
##
@@ -0,0 +1,300 @@
+/*
+ * 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.iceberg;
+
+import java.time.Instant;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+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;
+import org.apache.polaris.core.persistence.metrics.CommitMetricsRecord;
+import org.apache.polaris.core.persistence.metrics.ScanMetricsRecord;
+
+/**
+ * Converts Iceberg metrics reports to SPI record types using a fluent builder
API.
+ *
+ * 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.
+ *
+ * Example usage:
+ *
+ * {@code
+ * ScanMetricsRecord record = MetricsRecordConverter.forScanReport(scanReport)
+ * .catalogId(catalog.getId())
+ * .tableId(tableEntity.getId())
+ * .namespace(namespace)
+ * .build();
+ * }
+ */
+public final class MetricsRecordConverter {
+
+ private MetricsRecordConverter() {
+// Utility class
+ }
+
+ /**
+ * Creates a builder for converting a ScanReport to a ScanMetricsRecord.
+ *
+ * @param scanReport the Iceberg scan report
+ * @return builder for configuring the conversion
+ */
+ public static ScanReportBuilder forScanReport(ScanReport scanReport) {
+return new ScanReportBuilder(scanReport);
+ }
+
+ /**
+ * Creates a builder for converting a CommitReport to a CommitMetricsRecord.
+ *
+ * @param commitReport the Iceberg commit report
+ * @return builder for configuring the conversion
+ */
+ public static CommitReportBuilder forCommitReport(CommitReport commitReport)
{
+return new CommitReportBuilder(commitReport);
+ }
+
+ /** Builder for converting ScanReport to ScanMetricsRecord. */
+ public static final class ScanReportBuilder {
+private final ScanReport scanReport;
+private long catalogId;
+private long tableId;
+private List namespace = Collections.emptyList();
+private Instant timestamp;
+
+private ScanReportBuilder(ScanReport scanReport) {
+ this.scanReport = scanReport;
+}
+
+public ScanReportBuilder catalogId(long catalogId) {
+ this.catalogId = catalogId;
+ return this;
+}
+
+/**
+ * Sets the table entity ID.
+ *
+ * This is the internal Polaris entity ID for the table.
+ *
+ * @param tableId the table entity ID
+ * @return this builder
+ */
+public ScanReportBuilder tableId(long tableId) {
+ this.tableId = tableId;
+ return this;
+}
+
+/**
+ * Sets the namespace as a list of levels.
+ *
+ * @param namespace the namespace levels
+ * @return this builder
+ */
+public ScanReportBuilder namespace(List namespace) {
+ this.namespace = namespace != null ? namespace : Collections.emptyList();
+ return this;
+}
+
+/**
+ * Sets the timestamp for the metrics record.
+ *
+ * This should be the time the metrics report was received by the
server, which may differ
+ * from the time it was recorded by the client.
+ *
+ * @param timestamp the timestamp
+ * @return this builder
+ */
+public ScanReportBuilder timestamp(Instant timestamp) {
+ this.timestamp = timestamp;
+ return this;
+}
+
+public ScanMetricsRecord build() {
+ ScanMetricsResult metrics = scanReport.scanMetrics();
+ Map reportMetadata =
+ scanReport.metadata() != null ? scanReport.metadata() :
Collections.emptyMap();
+
+ return ScanMetricsRecord.builder()
+ .reportId(UUID.randomUUID().toString())
+ .catalogId(catalogId)
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
singhpk234 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2772723554
##
polaris-core/src/main/java/org/apache/polaris/core/metrics/iceberg/MetricsRecordConverter.java:
##
@@ -0,0 +1,300 @@
+/*
+ * 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.iceberg;
+
+import java.time.Instant;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+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;
+import org.apache.polaris.core.persistence.metrics.CommitMetricsRecord;
+import org.apache.polaris.core.persistence.metrics.ScanMetricsRecord;
+
+/**
+ * Converts Iceberg metrics reports to SPI record types using a fluent builder
API.
+ *
+ * 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.
+ *
+ * Example usage:
+ *
+ * {@code
+ * ScanMetricsRecord record = MetricsRecordConverter.forScanReport(scanReport)
+ * .catalogId(catalog.getId())
+ * .tableId(tableEntity.getId())
+ * .namespace(namespace)
+ * .build();
+ * }
+ */
+public final class MetricsRecordConverter {
+
+ private MetricsRecordConverter() {
+// Utility class
+ }
+
+ /**
+ * Creates a builder for converting a ScanReport to a ScanMetricsRecord.
+ *
+ * @param scanReport the Iceberg scan report
+ * @return builder for configuring the conversion
+ */
+ public static ScanReportBuilder forScanReport(ScanReport scanReport) {
+return new ScanReportBuilder(scanReport);
+ }
+
+ /**
+ * Creates a builder for converting a CommitReport to a CommitMetricsRecord.
+ *
+ * @param commitReport the Iceberg commit report
+ * @return builder for configuring the conversion
+ */
+ public static CommitReportBuilder forCommitReport(CommitReport commitReport)
{
+return new CommitReportBuilder(commitReport);
+ }
+
+ /** Builder for converting ScanReport to ScanMetricsRecord. */
+ public static final class ScanReportBuilder {
+private final ScanReport scanReport;
+private long catalogId;
+private long tableId;
+private List namespace = Collections.emptyList();
+private Instant timestamp;
+
+private ScanReportBuilder(ScanReport scanReport) {
+ this.scanReport = scanReport;
+}
+
+public ScanReportBuilder catalogId(long catalogId) {
+ this.catalogId = catalogId;
+ return this;
+}
+
+/**
+ * Sets the table entity ID.
+ *
+ * This is the internal Polaris entity ID for the table.
+ *
+ * @param tableId the table entity ID
+ * @return this builder
+ */
+public ScanReportBuilder tableId(long tableId) {
+ this.tableId = tableId;
+ return this;
+}
+
+/**
+ * Sets the namespace as a list of levels.
+ *
+ * @param namespace the namespace levels
+ * @return this builder
+ */
+public ScanReportBuilder namespace(List namespace) {
+ this.namespace = namespace != null ? namespace : Collections.emptyList();
+ return this;
+}
+
+/**
+ * Sets the timestamp for the metrics record.
+ *
+ * This should be the time the metrics report was received by the
server, which may differ
+ * from the time it was recorded by the client.
+ *
+ * @param timestamp the timestamp
+ * @return this builder
+ */
+public ScanReportBuilder timestamp(Instant timestamp) {
+ this.timestamp = timestamp;
+ return this;
+}
+
+public ScanMetricsRecord build() {
+ ScanMetricsResult metrics = scanReport.scanMetrics();
+ Map reportMetadata =
+ scanReport.metadata() != null ? scanReport.metadata() :
Collections.emptyMap();
+
+ return ScanMetricsRecord.builder()
+ .reportId(UUID.randomUUID().toString())
+ .catalogId(catalogId)
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2771688289
##
polaris-core/src/main/java/org/apache/polaris/core/metrics/iceberg/MetricsRecordConverter.java:
##
@@ -115,6 +116,20 @@ public ScanReportBuilder namespace(List namespace)
{
return this;
}
+/**
+ * Sets the timestamp for the metrics record.
+ *
+ * This should be the time the metrics report was received by the
server, which may differ
+ * from the time it was recorded by the client.
+ *
+ * @param timestamp the timestamp
+ * @return this builder
+ */
+public ScanReportBuilder timestamp(Instant timestamp) {
+ this.timestamp = timestamp;
Review Comment:
@dimas-b noticed this bug when I was rebasing
https://github.com/apache/polaris/pull/3385 to this.
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
dimas-b commented on PR #3616: URL: https://github.com/apache/polaris/pull/3616#issuecomment-3856894040 This PR is one week old and it looks like it did not attract attention from many contributors :sweat_smile: (thanks for your review, @evindj !). So, unless fresh comments appear, I'm planning to merge tomorrow. Since the new SPI is marked as `Beta`, if concerns are uncovered later, we'll address on `main`. -- 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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2771397003
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/MetricsPersistence.java:
##
@@ -0,0 +1,147 @@
+/*
+ * 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.
+ *
+ * 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.
+ *
+ * Implementations should be idempotent - writing the same reportId twice
should have no effect.
+ * Implementations that don't support metrics persistence can use {@link
#NOOP} which silently
+ * ignores write operations and returns empty pages for queries.
+ *
+ * Dependency Injection
+ *
+ * This interface is designed to be injected via CDI (Contexts and
Dependency Injection). The
+ * deployment module (e.g., {@code polaris-quarkus-service}) should provide a
{@code @Produces}
+ * method that creates the appropriate implementation based on the configured
persistence backend.
+ *
+ * Example producer:
+ *
+ * {@code
+ * @Produces
+ * @RequestScoped
+ * MetricsPersistence metricsPersistence(RealmContext realmContext,
PersistenceBackend backend) {
+ * if (backend.supportsMetrics()) {
+ * return backend.createMetricsPersistence(realmContext);
+ * }
+ * return MetricsPersistence.NOOP;
+ * }
+ * }
+ *
+ * Multi-Tenancy
+ *
+ * Realm context is not passed in the record objects. Implementations
should obtain the realm
+ * from the CDI-injected {@code RealmContext} at write/query time. This keeps
catalog-specific code
+ * from needing to manage realm concerns directly.
+ *
+ * Pagination
+ *
+ * Query methods use the standard Polaris pagination pattern with {@link
PageToken} for requests
+ * and {@link Page} for responses. This enables:
+ *
+ *
+ * Backend-specific cursor implementations (RDBMS offset, NoSQL
continuation tokens, etc.)
+ * Consistent pagination interface across all Polaris persistence APIs
+ * Efficient cursor-based pagination that works with large result sets
+ *
+ *
+ * The {@link ReportIdToken} provides a reference cursor implementation
based on report ID
+ * (UUID), but backends may use other cursor strategies internally.
+ *
+ * @see PageToken
+ * @see Page
+ * @see ReportIdToken
+ */
+public interface MetricsPersistence {
Review Comment:
Added @Beta annotation from Guava (com.google.common.annotations.Beta) to
all public types in the Metrics Persistence SPI package
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
dimas-b commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2771276882
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/MetricsPersistence.java:
##
@@ -0,0 +1,147 @@
+/*
+ * 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.
+ *
+ * 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.
+ *
+ * Implementations should be idempotent - writing the same reportId twice
should have no effect.
+ * Implementations that don't support metrics persistence can use {@link
#NOOP} which silently
+ * ignores write operations and returns empty pages for queries.
+ *
+ * Dependency Injection
+ *
+ * This interface is designed to be injected via CDI (Contexts and
Dependency Injection). The
+ * deployment module (e.g., {@code polaris-quarkus-service}) should provide a
{@code @Produces}
+ * method that creates the appropriate implementation based on the configured
persistence backend.
+ *
+ * Example producer:
+ *
+ * {@code
+ * @Produces
+ * @RequestScoped
+ * MetricsPersistence metricsPersistence(RealmContext realmContext,
PersistenceBackend backend) {
+ * if (backend.supportsMetrics()) {
+ * return backend.createMetricsPersistence(realmContext);
+ * }
+ * return MetricsPersistence.NOOP;
+ * }
+ * }
+ *
+ * Multi-Tenancy
+ *
+ * Realm context is not passed in the record objects. Implementations
should obtain the realm
+ * from the CDI-injected {@code RealmContext} at write/query time. This keeps
catalog-specific code
+ * from needing to manage realm concerns directly.
+ *
+ * Pagination
+ *
+ * Query methods use the standard Polaris pagination pattern with {@link
PageToken} for requests
+ * and {@link Page} for responses. This enables:
+ *
+ *
+ * Backend-specific cursor implementations (RDBMS offset, NoSQL
continuation tokens, etc.)
+ * Consistent pagination interface across all Polaris persistence APIs
+ * Efficient cursor-based pagination that works with large result sets
+ *
+ *
+ * The {@link ReportIdToken} provides a reference cursor implementation
based on report ID
+ * (UUID), but backends may use other cursor strategies internally.
+ *
+ * @see PageToken
+ * @see Page
+ * @see ReportIdToken
+ */
+public interface MetricsPersistence {
Review Comment:
Given that this is a new SPI and not fully implemented in all Persistence
backends yet, it would be nice to flag it as "experimental" for the sake of
clarity... but we do not have a common patterns for this in Polaris 😅
Perhaps we could try `com.google.common.annotations.Beta` in this particular
case... as a POC 🤔 WDYT?
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2766749220
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/MetricsQueryCriteria.java:
##
@@ -0,0 +1,164 @@
+/*
+ * 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 java.util.OptionalLong;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/**
+ * Query criteria for retrieving metrics reports.
+ *
+ * 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:
+ *
+ *
+ * Different backends to implement pagination in their optimal way
+ * Cursor-based pagination that works with both RDBMS and NoSQL backends
+ * Reuse of the existing Polaris pagination infrastructure
+ *
+ *
+ * Supported Query Patterns
+ *
+ *
+ * PatternFields UsedIndex Required
+ * By Table + TimecatalogId, tableId, startTime,
endTimeYes (OSS)
+ * By Time OnlystartTime, endTimePartial (timestamp
index)
+ *
+ *
+ * Additional query patterns (e.g., by trace ID) can be implemented by
persistence backends using
+ * the {@link #metadata()} filter map. Client-provided correlation data should
be stored in the
+ * metrics record's metadata map and can be filtered using the metadata
criteria.
+ *
+ * Pagination
+ *
+ * Pagination is handled via the {@link
org.apache.polaris.core.persistence.pagination.PageToken}
+ * passed to query methods. The token contains:
+ *
+ *
+ * {@code pageSize()} - Maximum number of results to return
+ * {@code value()} - Optional cursor token (e.g., {@link ReportIdToken})
for continuation
+ *
+ *
+ * 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 ID to filter by.
+ *
+ * This is the internal catalog entity ID. Callers should resolve catalog
names to IDs before
+ * querying, as catalog names can change over time.
+ */
+ OptionalLong catalogId();
+
+ /**
+ * Namespace to filter by.
+ *
+ * The namespace is represented as a list of levels to avoid ambiguity
when segments contain
+ * dots. An empty list means no namespace filter is applied.
+ */
+ List namespace();
Review Comment:
Makes sense.
I've removed namespace() from MetricsQueryCriteria. Queries are now by
catalogId + tableId only (both required). If users want to query by namespace,
the service layer should resolve namespace → table IDs using the current
catalog state, then query by those IDs.
I've kept namespace in MetricsRecordIdentity for storage/display purposes -
it's useful context when showing metrics to users, even though we don't query
by it.
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
dimas-b commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2766454944
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/MetricsQueryCriteria.java:
##
@@ -0,0 +1,164 @@
+/*
+ * 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 java.util.OptionalLong;
+import org.apache.polaris.immutables.PolarisImmutable;
+
+/**
+ * Query criteria for retrieving metrics reports.
+ *
+ * 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:
+ *
+ *
+ * Different backends to implement pagination in their optimal way
+ * Cursor-based pagination that works with both RDBMS and NoSQL backends
+ * Reuse of the existing Polaris pagination infrastructure
+ *
+ *
+ * Supported Query Patterns
+ *
+ *
+ * PatternFields UsedIndex Required
+ * By Table + TimecatalogId, tableId, startTime,
endTimeYes (OSS)
+ * By Time OnlystartTime, endTimePartial (timestamp
index)
+ *
+ *
+ * Additional query patterns (e.g., by trace ID) can be implemented by
persistence backends using
+ * the {@link #metadata()} filter map. Client-provided correlation data should
be stored in the
+ * metrics record's metadata map and can be filtered using the metadata
criteria.
+ *
+ * Pagination
+ *
+ * Pagination is handled via the {@link
org.apache.polaris.core.persistence.pagination.PageToken}
+ * passed to query methods. The token contains:
+ *
+ *
+ * {@code pageSize()} - Maximum number of results to return
+ * {@code value()} - Optional cursor token (e.g., {@link ReportIdToken})
for continuation
+ *
+ *
+ * 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 ID to filter by.
+ *
+ * This is the internal catalog entity ID. Callers should resolve catalog
names to IDs before
+ * querying, as catalog names can change over time.
+ */
+ OptionalLong catalogId();
+
+ /**
+ * Namespace to filter by.
+ *
+ * The namespace is represented as a list of levels to avoid ambiguity
when segments contain
+ * dots. An empty list means no namespace filter is applied.
+ */
+ List namespace();
Review Comment:
Sorry to cycle back on this... Now that we have IDs for catalog and table,
does it make sense to have name for namespace parts? 🤔
Is the idea to query all records for all tables under a namespace? This can
be confusing with table moves over time.
The namespace can still be a query parameter at the user level, but we could
just resolve namespace to table IDs using the current catalog state and then
query metrics persistence using table IDs. WDYT?
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
dimas-b commented on PR #3616: URL: https://github.com/apache/polaris/pull/3616#issuecomment-3850292135 @obelix74 : please rebased to fix CI -- 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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2766398017
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/MetricsQueryCriteria.java:
##
@@ -0,0 +1,146 @@
+/*
+ * 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.
+ *
+ * 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:
+ *
+ *
+ * Different backends to implement pagination in their optimal way
+ * Cursor-based pagination that works with both RDBMS and NoSQL backends
+ * Reuse of the existing Polaris pagination infrastructure
+ *
+ *
+ * Supported Query Patterns
+ *
+ *
+ * PatternFields UsedIndex Required
+ * By Table + TimecatalogName, namespace, tableName,
startTime, endTimeYes (OSS)
+ * By Time OnlystartTime, endTimePartial (timestamp
index)
+ *
+ *
+ * Additional query patterns (e.g., by trace ID) can be implemented by
persistence backends using
+ * the {@link #metadata()} filter map. Client-provided correlation data should
be stored in the
+ * metrics record's metadata map and can be filtered using the metadata
criteria.
+ *
+ * Pagination
+ *
+ * Pagination is handled via the {@link
org.apache.polaris.core.persistence.pagination.PageToken}
+ * passed to query methods. The token contains:
+ *
+ *
+ * {@code pageSize()} - Maximum number of results to return
+ * {@code value()} - Optional cursor token (e.g., {@link ReportIdToken})
for continuation
+ *
+ *
+ * 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 catalogName();
+
+ /** Namespace to filter by (dot-separated). */
+ Optional namespace();
+
+ /** Table name to filter by. */
+ Optional tableName();
+
+ // === Time Range ===
+
+ /** Start time for the query (inclusive). */
+ Optional startTime();
+
+ /** End time for the query (exclusive). */
+ Optional endTime();
+
+ // === Metadata Filtering ===
+
+ /**
+ * Metadata key-value pairs to filter by.
+ *
+ * This enables filtering metrics by client-provided correlation data
stored in the record's
+ * metadata map. For example, clients may include a trace ID in the metadata
that can be queried
+ * later.
+ *
+ * Note: Metadata filtering may require custom indexes depending on the
persistence backend.
+ * The OSS codebase provides basic support, but performance optimizations
may be needed for
+ * high-volume deployments.
+ */
+ java.util.Map metadata();
+
+ // === Factory Methods ===
+
+ /**
+ * Creates a new builder for MetricsQueryCriteria.
+ *
+ * @return a new builder instance
+ */
+ static ImmutableMetricsQueryCriteria.Builder builder() {
+return ImmutableMetricsQueryCriteria.builder();
+ }
+
+ /**
+ * Creates criteria for querying by table and time range.
+ *
+ * Pagination is handled separately via the {@code PageToken} parameter
to query methods.
+ *
+ * @param catalogName the catalog name
+ * @param namespace the namespace (dot-separated)
+ * @param tableName the table name
+ * @param startTime the start time (inclusive)
+ * @param endTime the end time (exclusive)
+ * @return the query criteria
+ */
+ static MetricsQueryCriteria forTable(
+ String catalogName, String namespace, String tableName, Instant
startTime, Instant endTime) {
Review Comment:
Updated forTable() to return a Builder pre-populated with catalogId and
tableId, allow
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2766397230
##
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.
+ *
+ * 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:
ok. Updated to use tableId (long) instead of tableName (String) in both
MetricsRecordIdentity and MetricsQueryCriteria. The caller now resolves table
entity ID before creating records, similar to how we handle catalogId.
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2766378976
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/MetricsQueryCriteria.java:
##
@@ -0,0 +1,146 @@
+/*
+ * 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.
+ *
+ * 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:
+ *
+ *
+ * Different backends to implement pagination in their optimal way
+ * Cursor-based pagination that works with both RDBMS and NoSQL backends
+ * Reuse of the existing Polaris pagination infrastructure
+ *
+ *
+ * Supported Query Patterns
+ *
+ *
+ * PatternFields UsedIndex Required
+ * By Table + TimecatalogName, namespace, tableName,
startTime, endTimeYes (OSS)
+ * By Time OnlystartTime, endTimePartial (timestamp
index)
+ *
+ *
+ * Additional query patterns (e.g., by trace ID) can be implemented by
persistence backends using
+ * the {@link #metadata()} filter map. Client-provided correlation data should
be stored in the
+ * metrics record's metadata map and can be filtered using the metadata
criteria.
+ *
+ * Pagination
+ *
+ * Pagination is handled via the {@link
org.apache.polaris.core.persistence.pagination.PageToken}
+ * passed to query methods. The token contains:
+ *
+ *
+ * {@code pageSize()} - Maximum number of results to return
+ * {@code value()} - Optional cursor token (e.g., {@link ReportIdToken})
for continuation
+ *
+ *
+ * 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 catalogName();
+
+ /** Namespace to filter by (dot-separated). */
+ Optional namespace();
Review Comment:
Updated both MetricsRecordIdentity and MetricsQueryCriteria to use
List for namespace. The persistence implementation will handle the
serialization format
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/MetricsQueryCriteria.java:
##
@@ -0,0 +1,146 @@
+/*
+ * 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.
+ *
+ * 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:
+ *
+ *
+ * Different backends to implement pagination in their optimal way
+ * Cursor-based pagination that works
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
dimas-b commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2766343275
##
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.
+ *
+ * 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:
I'm not completely sure about denormalizing table names into the metrics
table. With renames, a different table may appear under the same name in the
same namespace at different times... I tend to think it can easy become a
correctness issue at query time IDs seem more resilient... WDYT?
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
dimas-b commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2766321215
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/MetricsQueryCriteria.java:
##
@@ -0,0 +1,146 @@
+/*
+ * 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.
+ *
+ * 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:
+ *
+ *
+ * Different backends to implement pagination in their optimal way
+ * Cursor-based pagination that works with both RDBMS and NoSQL backends
+ * Reuse of the existing Polaris pagination infrastructure
+ *
+ *
+ * Supported Query Patterns
+ *
+ *
+ * PatternFields UsedIndex Required
+ * By Table + TimecatalogName, namespace, tableName,
startTime, endTimeYes (OSS)
+ * By Time OnlystartTime, endTimePartial (timestamp
index)
+ *
+ *
+ * Additional query patterns (e.g., by trace ID) can be implemented by
persistence backends using
+ * the {@link #metadata()} filter map. Client-provided correlation data should
be stored in the
+ * metrics record's metadata map and can be filtered using the metadata
criteria.
+ *
+ * Pagination
+ *
+ * Pagination is handled via the {@link
org.apache.polaris.core.persistence.pagination.PageToken}
+ * passed to query methods. The token contains:
+ *
+ *
+ * {@code pageSize()} - Maximum number of results to return
+ * {@code value()} - Optional cursor token (e.g., {@link ReportIdToken})
for continuation
+ *
+ *
+ * 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 catalogName();
+
+ /** Namespace to filter by (dot-separated). */
+ Optional namespace();
+
+ /** Table name to filter by. */
+ Optional tableName();
+
+ // === Time Range ===
+
+ /** Start time for the query (inclusive). */
+ Optional startTime();
+
+ /** End time for the query (exclusive). */
+ Optional endTime();
+
+ // === Metadata Filtering ===
+
+ /**
+ * Metadata key-value pairs to filter by.
+ *
+ * This enables filtering metrics by client-provided correlation data
stored in the record's
+ * metadata map. For example, clients may include a trace ID in the metadata
that can be queried
+ * later.
+ *
+ * Note: Metadata filtering may require custom indexes depending on the
persistence backend.
+ * The OSS codebase provides basic support, but performance optimizations
may be needed for
+ * high-volume deployments.
+ */
+ java.util.Map metadata();
+
+ // === Factory Methods ===
+
+ /**
+ * Creates a new builder for MetricsQueryCriteria.
+ *
+ * @return a new builder instance
+ */
+ static ImmutableMetricsQueryCriteria.Builder builder() {
+return ImmutableMetricsQueryCriteria.builder();
+ }
+
+ /**
+ * Creates criteria for querying by table and time range.
+ *
+ * Pagination is handled separately via the {@code PageToken} parameter
to query methods.
+ *
+ * @param catalogName the catalog name
+ * @param namespace the namespace (dot-separated)
+ * @param tableName the table name
+ * @param startTime the start time (inclusive)
+ * @param endTime the end time (exclusive)
+ * @return the query criteria
+ */
+ static MetricsQueryCriteria forTable(
+ String catalogName, String namespace, String tableName, Instant
startTime, Instant endTime) {
Review Comment:
nit: Would it be worth returning a Builder with just the table info and
adding time ran
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2766325915
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/MetricsQueryCriteria.java:
##
@@ -0,0 +1,146 @@
+/*
+ * 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.
+ *
+ * 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:
+ *
+ *
+ * Different backends to implement pagination in their optimal way
+ * Cursor-based pagination that works with both RDBMS and NoSQL backends
+ * Reuse of the existing Polaris pagination infrastructure
+ *
+ *
+ * Supported Query Patterns
+ *
+ *
+ * PatternFields UsedIndex Required
+ * By Table + TimecatalogName, namespace, tableName,
startTime, endTimeYes (OSS)
+ * By Time OnlystartTime, endTimePartial (timestamp
index)
+ *
+ *
+ * Additional query patterns (e.g., by trace ID) can be implemented by
persistence backends using
+ * the {@link #metadata()} filter map. Client-provided correlation data should
be stored in the
+ * metrics record's metadata map and can be filtered using the metadata
criteria.
+ *
+ * Pagination
+ *
+ * Pagination is handled via the {@link
org.apache.polaris.core.persistence.pagination.PageToken}
+ * passed to query methods. The token contains:
+ *
+ *
+ * {@code pageSize()} - Maximum number of results to return
+ * {@code value()} - Optional cursor token (e.g., {@link ReportIdToken})
for continuation
+ *
+ *
+ * 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 catalogName();
Review Comment:
This was replaced by `catalogId` based on feedback here.
https://github.com/apache/polaris/pull/3616#discussion_r2766276818
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2766322366
##
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.
+ *
+ * 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:
Agreed. I've updated the SPI to use separate namespace (String) and
tableName (String) primitives instead of TableIdentifier. This avoids Iceberg
dependencies in the Polaris SPI.
The conversion from TableIdentifier to these primitives now happens in
MetricsRecordConverter (in org.apache.polaris.core.metrics.iceberg package),
where Iceberg dependencies are already present.
Good point about catalog renames. I've removed catalogName from the SPI
records and now store only catalogId. The query criteria (MetricsQueryCriteria)
now uses catalogId (OptionalLong) instead of catalogName.
For tables, I'm keeping namespace and tableName as strings rather than
storing table entity IDs. While tables can also be renamed, users typically
query metrics by table name rather than by internal entity ID. This is a
pragmatic compromise - catalog renames are significant lifecycle events, while
table renames within the same namespace are more routine and less likely to
break historical queries.
Let me know if you'd prefer to also use table entity IDs instead.
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
dimas-b commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2766316855
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/MetricsQueryCriteria.java:
##
@@ -0,0 +1,146 @@
+/*
+ * 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.
+ *
+ * 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:
+ *
+ *
+ * Different backends to implement pagination in their optimal way
+ * Cursor-based pagination that works with both RDBMS and NoSQL backends
+ * Reuse of the existing Polaris pagination infrastructure
+ *
+ *
+ * Supported Query Patterns
+ *
+ *
+ * PatternFields UsedIndex Required
+ * By Table + TimecatalogName, namespace, tableName,
startTime, endTimeYes (OSS)
+ * By Time OnlystartTime, endTimePartial (timestamp
index)
+ *
+ *
+ * Additional query patterns (e.g., by trace ID) can be implemented by
persistence backends using
+ * the {@link #metadata()} filter map. Client-provided correlation data should
be stored in the
+ * metrics record's metadata map and can be filtered using the metadata
criteria.
+ *
+ * Pagination
+ *
+ * Pagination is handled via the {@link
org.apache.polaris.core.persistence.pagination.PageToken}
+ * passed to query methods. The token contains:
+ *
+ *
+ * {@code pageSize()} - Maximum number of results to return
+ * {@code value()} - Optional cursor token (e.g., {@link ReportIdToken})
for continuation
+ *
+ *
+ * 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 catalogName();
+
+ /** Namespace to filter by (dot-separated). */
+ Optional namespace();
+
+ /** Table name to filter by. */
+ Optional tableName();
+
+ // === Time Range ===
+
+ /** Start time for the query (inclusive). */
+ Optional startTime();
+
+ /** End time for the query (exclusive). */
+ Optional endTime();
+
+ // === Metadata Filtering ===
+
+ /**
+ * Metadata key-value pairs to filter by.
+ *
+ * This enables filtering metrics by client-provided correlation data
stored in the record's
+ * metadata map. For example, clients may include a trace ID in the metadata
that can be queried
+ * later.
+ *
+ * Note: Metadata filtering may require custom indexes depending on the
persistence backend.
+ * The OSS codebase provides basic support, but performance optimizations
may be needed for
+ * high-volume deployments.
+ */
+ java.util.Map metadata();
Review Comment:
nit: please use imports
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
dimas-b commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2766314919
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/MetricsQueryCriteria.java:
##
@@ -0,0 +1,146 @@
+/*
+ * 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.
+ *
+ * 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:
+ *
+ *
+ * Different backends to implement pagination in their optimal way
+ * Cursor-based pagination that works with both RDBMS and NoSQL backends
+ * Reuse of the existing Polaris pagination infrastructure
+ *
+ *
+ * Supported Query Patterns
+ *
+ *
+ * PatternFields UsedIndex Required
+ * By Table + TimecatalogName, namespace, tableName,
startTime, endTimeYes (OSS)
+ * By Time OnlystartTime, endTimePartial (timestamp
index)
+ *
+ *
+ * Additional query patterns (e.g., by trace ID) can be implemented by
persistence backends using
+ * the {@link #metadata()} filter map. Client-provided correlation data should
be stored in the
+ * metrics record's metadata map and can be filtered using the metadata
criteria.
+ *
+ * Pagination
+ *
+ * Pagination is handled via the {@link
org.apache.polaris.core.persistence.pagination.PageToken}
+ * passed to query methods. The token contains:
+ *
+ *
+ * {@code pageSize()} - Maximum number of results to return
+ * {@code value()} - Optional cursor token (e.g., {@link ReportIdToken})
for continuation
+ *
+ *
+ * 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 catalogName();
+
+ /** Namespace to filter by (dot-separated). */
+ Optional namespace();
Review Comment:
dot-separated value can be tricky to query if name segments contain dots
too... TBH, I'm not sure how well Polaris supports that now, but if escaping is
in use it can be very non-obvious.
Why not use a `List` here and let the impl. deal with persistent
representation?
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
dimas-b commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2766307155
##
polaris-core/src/main/java/org/apache/polaris/core/persistence/metrics/MetricsQueryCriteria.java:
##
@@ -0,0 +1,146 @@
+/*
+ * 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.
+ *
+ * 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:
+ *
+ *
+ * Different backends to implement pagination in their optimal way
+ * Cursor-based pagination that works with both RDBMS and NoSQL backends
+ * Reuse of the existing Polaris pagination infrastructure
+ *
+ *
+ * Supported Query Patterns
+ *
+ *
+ * PatternFields UsedIndex Required
+ * By Table + TimecatalogName, namespace, tableName,
startTime, endTimeYes (OSS)
+ * By Time OnlystartTime, endTimePartial (timestamp
index)
+ *
+ *
+ * Additional query patterns (e.g., by trace ID) can be implemented by
persistence backends using
+ * the {@link #metadata()} filter map. Client-provided correlation data should
be stored in the
+ * metrics record's metadata map and can be filtered using the metadata
criteria.
+ *
+ * Pagination
+ *
+ * Pagination is handled via the {@link
org.apache.polaris.core.persistence.pagination.PageToken}
+ * passed to query methods. The token contains:
+ *
+ *
+ * {@code pageSize()} - Maximum number of results to return
+ * {@code value()} - Optional cursor token (e.g., {@link ReportIdToken})
for continuation
+ *
+ *
+ * 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 catalogName();
Review Comment:
Catalog name is flagged as "convenience" in `MetricsRecordIdentity`, but may
be used for queries here... It's a bit of a contradiction IMHO... I think it's
ok to store it as a convenience attribute, if we do not have to query by it...
WDYT?
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2766288751
##
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.
+ *
+ * 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.
+ *
+ * Implementations should be idempotent - writing the same reportId twice
should have no effect.
+ * Implementations that don't support metrics persistence should return {@link
#NOOP}.
+ *
+ * Pagination
+ *
+ * Query methods use the standard Polaris pagination pattern with {@link
PageToken} for requests
+ * and {@link Page} for responses. This enables:
+ *
+ *
+ * Backend-specific cursor implementations (RDBMS offset, NoSQL
continuation tokens, etc.)
+ * Consistent pagination interface across all Polaris persistence APIs
+ * Efficient cursor-based pagination that works with large result sets
+ *
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * Returns an empty page if {@link #isSupported()} returns false.
+ *
+ * Example usage:
+ *
+ * {@code
+ * // First page
+ * PageToken pageToken = PageToken.fromLimit(100);
+ * Page page = persistence.queryScanReports(criteria,
pageToken);
+ *
+ * // Next page (if available)
+ * String nextPageToken = page.encodedResponseToken();
+ * if (nextPageToken != null) {
+ * pageToken = PageToken.build(nextPageToken, null, () -> true);
+ * Page nextPage =
persistence.queryScanReports(criteria, pageToken);
+ * }
+ * }
+ *
+ * @param criteria the query cr
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
dimas-b commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2766261231
##
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.
+ *
+ * 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.
+ *
+ * Implementations should be idempotent - writing the same reportId twice
should have no effect.
+ * Implementations that don't support metrics persistence should return {@link
#NOOP}.
+ *
+ * Pagination
+ *
+ * Query methods use the standard Polaris pagination pattern with {@link
PageToken} for requests
+ * and {@link Page} for responses. This enables:
+ *
+ *
+ * Backend-specific cursor implementations (RDBMS offset, NoSQL
continuation tokens, etc.)
+ * Consistent pagination interface across all Polaris persistence APIs
+ * Efficient cursor-based pagination that works with large result sets
+ *
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * Returns an empty page if {@link #isSupported()} returns false.
+ *
+ * Example usage:
+ *
+ * {@code
+ * // First page
+ * PageToken pageToken = PageToken.fromLimit(100);
+ * Page page = persistence.queryScanReports(criteria,
pageToken);
+ *
+ * // Next page (if available)
+ * String nextPageToken = page.encodedResponseToken();
+ * if (nextPageToken != null) {
+ * pageToken = PageToken.build(nextPageToken, null, () -> true);
+ * Page nextPage =
persistence.queryScanReports(criteria, pageToken);
+ * }
+ * }
+ *
+ * @param criteria the query cri
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
dimas-b commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2766276818
##
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.
+ *
+ * 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:
Good points about heavy objects in an "Immutable" class :thinking: However,
catalog name can generally
[change](https://github.com/apache/polaris/blob/d95bd6b1e3d77b5db7769e866c0474f08accad2c/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java#L313)
over time, right? Why would we persist it in a metrics record? Querying a
time-series over a changeable attribute can be very challenging :thinking:
Why not store (unique) IDs only inside Metrics Records? Granted, queries
will have to resolve names to IDs first, but that can be done using the latest
state of the catalog
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
dimas-b commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2766267423
##
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.
+ *
+ * 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:
I'd prefer to avoid Iceberg classes like `TableIdentifier` in a Polaris SPI.
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
dimas-b commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2766263710
##
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.
+ *
+ * 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:
yes. thx!
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
dimas-b commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2766261231
##
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.
+ *
+ * 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.
+ *
+ * Implementations should be idempotent - writing the same reportId twice
should have no effect.
+ * Implementations that don't support metrics persistence should return {@link
#NOOP}.
+ *
+ * Pagination
+ *
+ * Query methods use the standard Polaris pagination pattern with {@link
PageToken} for requests
+ * and {@link Page} for responses. This enables:
+ *
+ *
+ * Backend-specific cursor implementations (RDBMS offset, NoSQL
continuation tokens, etc.)
+ * Consistent pagination interface across all Polaris persistence APIs
+ * Efficient cursor-based pagination that works with large result sets
+ *
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * Returns an empty page if {@link #isSupported()} returns false.
+ *
+ * Example usage:
+ *
+ * {@code
+ * // First page
+ * PageToken pageToken = PageToken.fromLimit(100);
+ * Page page = persistence.queryScanReports(criteria,
pageToken);
+ *
+ * // Next page (if available)
+ * String nextPageToken = page.encodedResponseToken();
+ * if (nextPageToken != null) {
+ * pageToken = PageToken.build(nextPageToken, null, () -> true);
+ * Page nextPage =
persistence.queryScanReports(criteria, pageToken);
+ * }
+ * }
+ *
+ * @param criteria the query cri
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on PR #3616: URL: https://github.com/apache/polaris/pull/3616#issuecomment-3844982095 @dimas-b Thank you again for the detailed review. I have addressed all your questions. There are a few open items. * Did not use `CatalogEntity` - explained why. * Added comments about query sub-interfaces and separate query methods. I will follow your lead when you respond. -- 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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2761917119
##
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.
+ *
+ * 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:
For table identification: Using Iceberg's TableIdentifier instead of
separate namespace/tableName strings. This encapsulates both namespace path and
table name, aligns with Iceberg patterns, and is consistent with
`TableLikeEntity.getTableIdentifier()`.
For catalog identification: Keeping catalogId (long) and catalogName
(String) as separate primitives rather than using CatalogEntity.
Rationale:
1. Heavyweight object: CatalogEntity extends PolarisEntity and includes
storage config, properties, internal properties, timestamps, grant records
version - none relevant for metrics identification
2. Immutables incompatibility: CatalogEntity is a class wrapping
PolarisBaseEntity, not a @PolarisImmutable interface. The Immutables annotation
processor cannot generate proper builders that include it - I can't figure out
a solution for this.
3. Serialization complexity: CatalogEntity uses @JsonCreator/@JsonProperty
annotations that don't integrate cleanly with Immutables-generated classes
4. Minimal data needed: For metrics records, we only need catalog ID (for
foreign key relationships) and name (for display/query convenience)
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2761900610
##
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.
+ *
+ * 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:
Moved it to `org.apache.polaris.core.metrics.iceberg` package. Does that
work?
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2761897638
##
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.
+ *
+ * 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.
+ *
+ * Implementations should be idempotent - writing the same reportId twice
should have no effect.
+ * Implementations that don't support metrics persistence should return {@link
#NOOP}.
+ *
+ * Pagination
+ *
+ * Query methods use the standard Polaris pagination pattern with {@link
PageToken} for requests
+ * and {@link Page} for responses. This enables:
+ *
+ *
+ * Backend-specific cursor implementations (RDBMS offset, NoSQL
continuation tokens, etc.)
+ * Consistent pagination interface across all Polaris persistence APIs
+ * Efficient cursor-based pagination that works with large result sets
+ *
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * Returns an empty page if {@link #isSupported()} returns false.
+ *
+ * Example usage:
+ *
+ * {@code
+ * // First page
+ * PageToken pageToken = PageToken.fromLimit(100);
+ * Page page = persistence.queryScanReports(criteria,
pageToken);
+ *
+ * // Next page (if available)
+ * String nextPageToken = page.encodedResponseToken();
+ * if (nextPageToken != null) {
+ * pageToken = PageToken.build(nextPageToken, null, () -> true);
+ * Page nextPage =
persistence.queryScanReports(criteria, pageToken);
+ * }
+ * }
+ *
+ * @param criteria the query cr
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2761891835
##
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.
+ *
+ * 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.
+ *
+ * Implementations should be idempotent - writing the same reportId twice
should have no effect.
+ * Implementations that don't support metrics persistence should return {@link
#NOOP}.
+ *
+ * Pagination
+ *
+ * Query methods use the standard Polaris pagination pattern with {@link
PageToken} for requests
+ * and {@link Page} for responses. This enables:
+ *
+ *
+ * Backend-specific cursor implementations (RDBMS offset, NoSQL
continuation tokens, etc.)
+ * Consistent pagination interface across all Polaris persistence APIs
+ * Efficient cursor-based pagination that works with large result sets
+ *
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * Returns an empty page if {@link #isSupported()} returns false.
+ *
+ * Example usage:
+ *
+ * {@code
+ * // First page
+ * PageToken pageToken = PageToken.fromLimit(100);
+ * Page page = persistence.queryScanReports(criteria,
pageToken);
+ *
+ * // Next page (if available)
+ * String nextPageToken = page.encodedResponseToken();
+ * if (nextPageToken != null) {
+ * pageToken = PageToken.build(nextPageToken, null, () -> true);
+ * Page nextPage =
persistence.queryScanReports(criteria, pageToken);
+ * }
+ * }
+ *
+ * @param criteria the query cr
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2761887074
##
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.
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * 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 reportTraceId();
Review Comment:
Deleted.
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2761885073
##
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).
+ *
+ * This token enables cursor-based pagination for metrics queries across
different storage
+ * backends. The report ID is used as the cursor because it is:
+ *
+ *
+ * Guaranteed unique across all reports
+ * Present in both scan and commit metrics records
+ * Stable (doesn't change over time)
+ *
+ *
+ * Each backend implementation can use this cursor value to implement
efficient pagination in
+ * whatever way is optimal for that storage system:
+ *
+ *
+ * RDBMS: {@code WHERE report_id > :lastReportId ORDER BY report_id}
+ * NoSQL: Use report ID as partition/sort key cursor
+ * Time-series: Combine with timestamp for efficient range scans
Review Comment:
Updated ReportIdToken Javadoc. Clarified it's a reference implementation,
not required by the SPI. Backends can implement their own Token subclass
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2761884273
##
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.
+ *
+ * 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:
Replaced it with a builder pattern and deleted MetricsContext.
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2761880552
##
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.
+ *
+ * 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:
+ *
+ *
+ * Different backends to implement pagination in their optimal way
+ * Cursor-based pagination that works with both RDBMS and NoSQL backends
+ * Reuse of the existing Polaris pagination infrastructure
+ *
+ *
+ * Supported Query Patterns
+ *
+ *
+ * PatternFields UsedIndex Required
+ * By Table + TimecatalogName, namespace, tableName,
startTime, endTimeYes (OSS)
+ * By Client Trace IDreportTraceIdNo (custom
deployment)
+ * By Time OnlystartTime, endTimePartial (timestamp
index)
+ *
+ *
+ * Pagination
+ *
+ * Pagination is handled via the {@link
org.apache.polaris.core.persistence.pagination.PageToken}
+ * passed to query methods. The token contains:
+ *
+ *
+ * {@code pageSize()} - Maximum number of results to return
+ * {@code value()} - Optional cursor token (e.g., {@link ReportIdToken})
for continuation
+ *
+ *
+ * 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 catalogName();
+
+ /** Namespace to filter by (dot-separated). */
+ Optional namespace();
+
+ /** Table name to filter by. */
+ Optional tableName();
+
+ // === Time Range ===
+
+ /** Start time for the query (inclusive). */
+ Optional startTime();
+
+ /** End time for the query (exclusive). */
+ Optional endTime();
+
+ // === Correlation ===
+
+ /**
+ * Client-provided trace ID to filter by (from report metadata).
+ *
+ * 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.
+ *
+ * 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 reportTraceId();
Review Comment:
Removed reportTraceId field. Removed from ScanMetricsRecord,
CommitMetricsRecord, and MetricsQueryCriteria. Added metadata() map to
MetricsRecordIdentity for flexible key-value storage if needed
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2761879321
##
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.
+ *
+ * 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.
+ *
+ * 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:
Deleted MetricsPersistenceFactory. CDI producers in the deployment module
handle implementation selection
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2761878064
##
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.
+ *
+ * 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.
+ *
+ * Implementations should be idempotent - writing the same reportId twice
should have no effect.
+ * Implementations that don't support metrics persistence should return {@link
#NOOP}.
+ *
+ * Pagination
+ *
+ * Query methods use the standard Polaris pagination pattern with {@link
PageToken} for requests
+ * and {@link Page} for responses. This enables:
+ *
+ *
+ * Backend-specific cursor implementations (RDBMS offset, NoSQL
continuation tokens, etc.)
+ * Consistent pagination interface across all Polaris persistence APIs
+ * Efficient cursor-based pagination that works with large result sets
+ *
+ *
+ * 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.
+ *
+ * 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:
Deleted.
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2761877735
##
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.
+ *
+ * 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:
Changed catalogId from String to long. Now matches PolarisEntityCore#getId()
as suggested
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2761876976
##
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.
+ *
+ * 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:
Created MetricsRecordIdentity base interface
New interface in org.apache.polaris.core.persistence.metrics with common
fields:
• reportId(), catalogId(), catalogName(), namespace(), tableName(),
timestamp(), metadata()
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on code in PR #3616:
URL: https://github.com/apache/polaris/pull/3616#discussion_r2761875784
##
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.
+ *
+ * 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:
Thank you @dimas-b for the thorough review. I've addressed all the feedback:
1. Removed realmId from metrics records
• Realm context is now obtained from CDI-injected RealmContext at
persistence time
• Added documentation in MetricsPersistence.java explaining this pattern
--
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]
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
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.
+ *
+ * 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.
+ *
+ * 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.
+ *
+ * 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 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.
+ *
+ * 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/met
Re: [PR] feat(metrics): Add MetricsPersistence SPI for backend-agnostic metrics storage (#3337) [polaris]
obelix74 commented on PR #3616: URL: https://github.com/apache/polaris/pull/3616#issuecomment-3820923736 @dimas-b This is the PR for the SPI, as you suggested. This contains the document feedback incorporated. -- 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]
