singhpk234 commented on code in PR #3468:
URL: https://github.com/apache/polaris/pull/3468#discussion_r2717817876
##########
runtime/service/src/main/java/org/apache/polaris/service/reporting/PolarisMetricsReporter.java:
##########
@@ -18,9 +18,39 @@
*/
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 can be annotated with appropriate {@code Quarkus}
scope 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>Implementations can inject other CDI beans for context.
+ *
+ * @see DefaultMetricsReporter
+ * @see MetricsReportingConfiguration
+ */
public interface PolarisMetricsReporter {
- public void reportMetric(String catalogName, TableIdentifier table,
MetricsReport metricsReport);
Review Comment:
Thats fair, though in this case we are forcing the upgrades to have this new
interface implementation when having the old api was not hurting ? taking the
case LOGGERs can be configured to implicitly log timestamp additionally and the
way we have reporting wired if i wanna do custom one can just do timestamp
internally rather than requesting from the signature ?
```
metricsReporter.reportMetric(
catalogName, tableIdentifier, reportMetricsRequest.report(),
clock.instant());
```
it not the time when the request hit the server but its the time when the
reportMetric is called so does it matter of i do this vs
reportMetric(catalogName, tableIdentifier, reportMetricsRequest.report())) {
timestamp = clock.instant()
}
with that being said i think its fine if we wanna move forward :), but above
is my thought process. Please move forward @dimas-b trust your judgement here !
--
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]
