dimas-b commented on code in PR #3468:
URL: https://github.com/apache/polaris/pull/3468#discussion_r2709711745
##########
CHANGELOG.md:
##########
@@ -52,6 +52,10 @@ request adding CHANGELOG notes for breaking (!) changes and
possibly other secti
- The EclipseLink Persistence implementation has been completely removed.
- The default request ID header name has changed from `Polaris-Request-Id` to
`X-Request-ID`.
- The (Before/After)CommitTableEvent has been removed.
+- The `PolarisMetricsReporter.reportMetric()` method signature has been
extended to include a
+ `receivedTimestamp` parameter of type `java.time.Instant`. A
backward-compatible default method
+ is provided for existing implementations, but custom implementations should
migrate to the new
+ signature.
Review Comment:
Please also see my comment on the actual SPI change. I do not think we
provide backward compatibility in this case (nor do we have to).
##########
runtime/service/src/main/java/org/apache/polaris/service/reporting/PolarisMetricsReporter.java:
##########
@@ -18,9 +18,84 @@
*/
package org.apache.polaris.service.reporting;
+import java.time.Instant;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.metrics.MetricsReport;
+/**
+ * SPI interface for reporting Iceberg metrics received by Polaris.
+ *
+ * <p>Implementations can be used to send metrics to external systems for
analysis and monitoring.
+ * Custom implementations must be annotated with {@link
+ * jakarta.enterprise.context.ApplicationScoped @ApplicationScoped} and {@link
Review Comment:
Why do we have to force `ApplicationScoped` here? I believe it's more
natural for `PolarisMetricsReporter` implementations to be request-scoped (in
order to potentially benefit from other request-scoped data like
`PolarisPrincipal`).
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -805,6 +805,7 @@ public Response reportMetrics(
ReportMetricsRequest reportMetricsRequest,
RealmContext realmContext,
SecurityContext securityContext) {
+ // Delegate directly to the underlying service.
Review Comment:
nit: isn't it obvious from the code?
##########
runtime/service/src/main/java/org/apache/polaris/service/reporting/PolarisMetricsReporter.java:
##########
@@ -18,9 +18,84 @@
*/
package org.apache.polaris.service.reporting;
+import java.time.Instant;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.metrics.MetricsReport;
+/**
+ * SPI interface for reporting Iceberg metrics received by Polaris.
+ *
+ * <p>Implementations can be used to send metrics to external systems for
analysis and monitoring.
+ * Custom implementations must be annotated with {@link
+ * jakarta.enterprise.context.ApplicationScoped @ApplicationScoped} and {@link
+ * io.smallrye.common.annotation.Identifier @Identifier("my-reporter-type")}
for CDI discovery.
+ *
+ * <p>The implementation to use is selected via the {@code
polaris.iceberg-metrics.reporting.type}
+ * configuration property, which defaults to {@code "default"}.
+ *
+ * <p>Custom implementations that need access to request-scoped context should
inject the
+ * appropriate CDI beans rather than expecting this data to be passed as
parameters:
+ *
+ * <ul>
+ * <li>{@code RealmContext} - for realm/tenant information
+ * <li>{@code io.opentelemetry.api.trace.Span} - for OpenTelemetry
trace/span IDs (inject via
+ * {@code @Inject Span span} or use {@code Span.current()})
+ * <li>{@code PolarisPrincipalHolder} - for authenticated principal
information
+ * </ul>
+ *
+ * <p>Example implementation with OpenTelemetry correlation:
+ *
+ * <pre>{@code
+ * @ApplicationScoped
+ * @Identifier("custom")
+ * public class CustomMetricsReporter implements PolarisMetricsReporter {
+ *
+ * @Inject RealmContext realmContext;
+ * @Inject Span span; // or use Span.current()
+ *
+ * @Override
+ * public void reportMetric(
+ * String catalogName, TableIdentifier table, MetricsReport
metricsReport,
+ * Instant receivedTimestamp) {
+ * SpanContext spanContext = span.getSpanContext();
+ * String traceId = spanContext.isValid() ? spanContext.getTraceId() :
null;
+ * // Send metrics to external system with trace correlation
+ * }
+ * }
+ * }</pre>
+ *
+ * @see DefaultMetricsReporter
+ * @see MetricsReportingConfiguration
+ */
public interface PolarisMetricsReporter {
- public void reportMetric(String catalogName, TableIdentifier table,
MetricsReport metricsReport);
+
+ /**
+ * Reports an Iceberg metrics report for a specific table.
+ *
+ * @param catalogName the name of the catalog containing the table
+ * @param table the identifier of the table the metrics are for
+ * @param metricsReport the Iceberg metrics report (e.g., {@link
+ * org.apache.iceberg.metrics.ScanReport} or {@link
org.apache.iceberg.metrics.CommitReport})
+ * @param receivedTimestamp the timestamp when the metrics were received by
Polaris
+ */
+ void reportMetric(
+ String catalogName,
+ TableIdentifier table,
+ MetricsReport metricsReport,
+ Instant receivedTimestamp);
+
+ /**
+ * Reports an Iceberg metrics report for a specific table.
+ *
+ * @param catalogName the name of the catalog containing the table
+ * @param table the identifier of the table the metrics are for
+ * @param metricsReport the Iceberg metrics report
+ * @deprecated Use {@link #reportMetric(String, TableIdentifier,
MetricsReport, Instant)} instead.
+ * This method is provided for backward compatibility and will be
removed in a future release.
+ */
+ @Deprecated
+ default void reportMetric(
+ String catalogName, TableIdentifier table, MetricsReport metricsReport) {
+ reportMetric(catalogName, table, metricsReport, Instant.now());
+ }
Review Comment:
I'm not sure what this method achieves 🤔 Old implementation will have an
override for it already, but will be missing an impl. for the new
four-parameter `reportMetric()` method, so it's going to be a compilation error
anyway (which is ok per our evolution guidelines).
Polaris will not call this method, either (unless I'm mistaken).
Third-party code is not supposed to call this SPI directly (only implement
it to receive calls from Polaris).
... so all in all this looks like dead code 🤔
##########
runtime/service/src/main/java/org/apache/polaris/service/reporting/PolarisMetricsReporter.java:
##########
@@ -18,9 +18,84 @@
*/
package org.apache.polaris.service.reporting;
+import java.time.Instant;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.metrics.MetricsReport;
+/**
+ * SPI interface for reporting Iceberg metrics received by Polaris.
+ *
+ * <p>Implementations can be used to send metrics to external systems for
analysis and monitoring.
+ * Custom implementations must be annotated with {@link
+ * jakarta.enterprise.context.ApplicationScoped @ApplicationScoped} and {@link
Review Comment:
TBH, I'm not sure we need to go into too much CDI details in this javadoc
(the example impl. below is fine)... otherwise, we'd have to do the same for
may other CDI plugin points 😅 Developers integrating at this level can be
expected to know CDI, I guess 😉
Would it be sufficient to method the config-based bean resolution?
Downstream builds will be free to choose whatever CDI scope makes sense for
their use cases.
CDI delegation (chaining) to other reported types would also be possible in
downstream builds.
--
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]
