adutra commented on code in PR #3468:
URL: https://github.com/apache/polaris/pull/3468#discussion_r2707882844
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -64,18 +65,21 @@ public class IcebergRestCatalogEventServiceDelegator
@Inject PolarisEventListener polarisEventListener;
@Inject PolarisEventMetadataFactory eventMetadataFactory;
@Inject CatalogPrefixParser prefixParser;
+ @Inject RealmConfig realmConfig;
Review Comment:
Changes in this file seem unnecessary.
##########
runtime/service/src/main/java/org/apache/polaris/service/config/ServiceProducers.java:
##########
@@ -429,6 +431,46 @@ public void closeTaskExecutor(@Disposes
@Identifier("task-executor") ManagedExec
executor.close();
}
+ /**
+ * Produces the current OpenTelemetry {@link SpanContext} for the request.
+ *
+ * <p>This allows custom {@link PolarisMetricsReporter} implementations to
access the current
+ * trace and span IDs for correlation with external monitoring systems.
+ *
+ * <p>Example usage in a custom metrics reporter:
+ *
+ * <pre>{@code
+ * @ApplicationScoped
+ * @Identifier("custom")
+ * public class CustomMetricsReporter implements PolarisMetricsReporter {
+ *
+ * @Inject SpanContext spanContext;
+ *
+ * @Override
+ * public void reportMetric(String catalogName, TableIdentifier table,
+ * MetricsReport metricsReport, long timestampMs) {
+ * String traceId = spanContext.isValid() ? spanContext.getTraceId() :
null;
+ * String spanId = spanContext.isValid() ? spanContext.getSpanId() :
null;
+ * // Forward metrics with trace correlation...
+ * }
+ * }
+ * }</pre>
+ *
+ * @return the current span context, or {@link SpanContext#getInvalid()} if
no span is active
+ */
+ @Produces
+ @RequestScoped
+ public SpanContext currentSpanContext() {
+ Span currentSpan = Span.current();
+ if (currentSpan != null) {
+ SpanContext spanContext = currentSpan.getSpanContext();
+ if (spanContext != null) {
+ return spanContext;
+ }
+ }
+ return SpanContext.getInvalid();
Review Comment:
Imho this bean is unnecessary.
In fact the static call to `Span.current()` is already "wired" by Quarkus to
a machinery to retrieve the request-scoped `Span` associated with the current
CDI context. Just use `Span.current()` whenever you need it, and it will give
you the right `Span`.
I think that's what @dimas-b meant by "handle OTel via its natural context
objects."
Alternatively, Quarkus already allows you to inject `Span` objects out of
the box:
https://quarkus.io/guides/opentelemetry-tracing#available-opentelemetry-cdi-injections.
##########
runtime/service/src/main/java/org/apache/polaris/service/reporting/PolarisMetricsReporter.java:
##########
@@ -32,23 +32,30 @@
* <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 (such
as realm information
- * or principal details) should inject the appropriate CDI beans (e.g., {@code
RealmContext}, {@code
- * PolarisPrincipalHolder}) rather than expecting this data to be passed as
parameters.
+ * <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:
*
- * <p>Example implementation:
+ * <ul>
+ * <li>{@code RealmContext} - for realm/tenant information
+ * <li>{@code io.opentelemetry.api.trace.SpanContext} - for OpenTelemetry
trace/span IDs
+ * <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 SpanContext spanContext;
Review Comment:
See my other comment. It's no bad practice to call `Span.current()`.
Alternatively, injecting a `Span` works out of the box too (but not
`SpanContext`).
##########
runtime/service/src/main/java/org/apache/polaris/service/reporting/PolarisMetricsReporter.java:
##########
@@ -22,5 +22,6 @@
import org.apache.iceberg.metrics.MetricsReport;
public interface PolarisMetricsReporter {
- public void reportMetric(String catalogName, TableIdentifier table,
MetricsReport metricsReport);
+ void reportMetric(
+ String catalogName, TableIdentifier table, MetricsReport metricsReport,
long timestampMs);
Review Comment:
I also agree that this timestamp doesn't look super helpful, but why not.
But I also find it a bit low-level. Why not pass a full `Instant` instead?
##########
runtime/service/src/main/java/org/apache/polaris/service/reporting/MetricsProcessor.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * 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.service.reporting;
+
+/**
+ * Interface for processing Iceberg metrics reports in Polaris.
+ *
+ * <p>This interface provides a pluggable mechanism for handling metrics
reports from Iceberg table
+ * operations. Implementations can persist metrics to various backends,
forward them to external
+ * systems, or perform custom processing.
+ *
+ * <p>Processors are discovered via CDI using the {@link
io.smallrye.common.annotation.Identifier}
+ * annotation. Custom processors can be implemented and registered by
annotating them with
+ * {@code @ApplicationScoped} and {@code @Identifier("custom-name")}.
+ *
+ * <p>Available built-in processors:
+ *
+ * <ul>
+ * <li>{@code noop} - Discards all metrics (default)
+ * <li>{@code logging} - Logs metrics to console for debugging
+ * <li>{@code persistence} - Persists to dedicated metrics tables
+ * </ul>
+ *
+ * <p>Example configuration:
+ *
+ * <pre>
+ * polaris:
+ * metrics:
+ * processor:
+ * type: persistence
+ * </pre>
+ *
+ * <p>Custom implementations should be annotated with:
+ *
+ * <pre>
+ * {@literal @}ApplicationScoped
+ * {@literal @}Identifier("custom-processor")
+ * public class CustomMetricsProcessor implements MetricsProcessor {
+ * {@literal @}Override
+ * public void process(MetricsProcessingContext context) {
+ * // implementation
+ * }
+ * }
+ * </pre>
+ *
+ * @see MetricsProcessingContext
+ * @see MetricsProcessorConfiguration
+ */
+public interface MetricsProcessor {
+
+ /**
+ * Process a metrics report with full context information.
+ *
+ * <p>Implementations should handle exceptions gracefully and not throw
exceptions that would
+ * disrupt the metrics reporting flow. Errors should be logged and metrics
about processing
+ * failures should be emitted.
+ *
+ * @param context the complete context for metrics processing
+ */
+ void process(MetricsProcessingContext context);
Review Comment:
Update: in fact even the SPI interface is backwards-compatible; this is nice
👍
--
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]
