adutra commented on code in PR #2887:
URL: https://github.com/apache/polaris/pull/2887#discussion_r2460365883
##########
runtime/service/src/main/java/org/apache/polaris/service/reporting/LoggingMetricsReporter.java:
##########
@@ -0,0 +1,22 @@
+package org.apache.polaris.service.reporting;
+
+import io.smallrye.common.annotation.Identifier;
+import jakarta.enterprise.context.RequestScoped;
+import org.apache.iceberg.rest.requests.ReportMetricsRequest;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@RequestScoped
+@Identifier("logging")
+public class LoggingMetricsReporter implements MetricsReporter {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(LoggingMetricsReporter.class);
+
+ @Override
+ public void reportMetric(
+ String prefix, String namespace, String table, ReportMetricsRequest
reportMetricsRequest) {
+ LOGGER.info(prefix);
Review Comment:
It would be better to consolidate all information into a single message.
This is because the four lines are not guaranteed to appear consecutively in
the logs.
##########
runtime/service/src/main/java/org/apache/polaris/service/reporting/MetricsReporter.java:
##########
@@ -0,0 +1,8 @@
+package org.apache.polaris.service.reporting;
+
+import org.apache.iceberg.rest.requests.ReportMetricsRequest;
+
+public interface MetricsReporter {
Review Comment:
Iceberg already has a `MetricsReporter` and a `LoggingMetricsReporter`. I
suppose you created new ones because you wanted to pass the warehouse and
namespace names as well? In this case would it be possible to _extend_
Iceberg's `MetricsReporter`?
##########
runtime/service/src/main/java/org/apache/polaris/service/reporting/MetricsReporter.java:
##########
@@ -0,0 +1,8 @@
+package org.apache.polaris.service.reporting;
+
+import org.apache.iceberg.rest.requests.ReportMetricsRequest;
+
+public interface MetricsReporter {
+ public void reportMetric(
+ String prefix, String namespace, String table, ReportMetricsRequest
reportMetricsRequest);
Review Comment:
1. I would rather rename `prefix` to `warehouse` or `catalogName` since
`prefix` is a REST-specific notion.
1. But you should to call
`prefixParser.prefixToCatalogName(realmContext, prefix)` to get the catalog
name from the prefix.
2. For a richer API, I would use `TableIdentifier` rather than 2 strings
(namespace + table). It's very easy to get a `TableIdentifier`:
```java
Namespace ns = decodeNamespace(namespace);
TableIdentifier tableIdentifier = TableIdentifier.of(ns,
RESTUtil.decodeString(table));
```
##########
runtime/service/src/test/java/org/apache/polaris/service/reporting/MetricsReportingConfigurationTest.java:
##########
@@ -0,0 +1,46 @@
+package org.apache.polaris.service.reporting;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import com.google.common.collect.ImmutableMap;
+import io.quarkus.test.junit.QuarkusTest;
+import io.quarkus.test.junit.TestProfile;
+import jakarta.inject.Inject;
+import java.util.Map;
+import org.junit.jupiter.api.Test;
+
+@QuarkusTest
+public class MetricsReportingConfigurationTest {
+ @Inject MetricsReportingConfiguration metricsConfig;
+
+ @Test
+ @TestProfile(MetricsReportingConfiguration.DefaultProfile.class)
+ void testDefault() {
Review Comment:
These tests are rather useless as they are basically testing SmallRye Config.
What would be more meaningful is a real test that invokes the IRC metrics
endpoint and verifies that metrics were collected.
##########
runtime/service/src/main/java/org/apache/polaris/service/reporting/LoggingMetricsReporter.java:
##########
@@ -0,0 +1,22 @@
+package org.apache.polaris.service.reporting;
+
+import io.smallrye.common.annotation.Identifier;
+import jakarta.enterprise.context.RequestScoped;
+import org.apache.iceberg.rest.requests.ReportMetricsRequest;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@RequestScoped
+@Identifier("logging")
+public class LoggingMetricsReporter implements MetricsReporter {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(LoggingMetricsReporter.class);
+
+ @Override
+ public void reportMetric(
Review Comment:
I'm not sure we need a distinct logging reporter. The default reporter could
log the metrics, then users could simply adjust their log levels and decide
whether they want the metrics to appear in the logs or not (by default, they
wouldn't be logged). WDYT?
##########
runtime/service/src/main/java/org/apache/polaris/service/reporting/DefaultMetricsReporter.java:
##########
@@ -0,0 +1,19 @@
+package org.apache.polaris.service.reporting;
+
+import io.smallrye.common.annotation.Identifier;
+import jakarta.enterprise.context.RequestScoped;
+import org.apache.iceberg.rest.requests.ReportMetricsRequest;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@RequestScoped
+@Identifier("default")
+public class DefaultMetricsReporter implements MetricsReporter {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(DefaultMetricsReporter.class);
+
+ @Override
+ public void reportMetric(
+ String prefix, String namespace, String table, ReportMetricsRequest
reportMetricsRequest) {
+ // Do Nothing
Review Comment:
1. Would it make sense to define and maintain a set of Micrometer metrics
and increment them here?
2. Would it make sense to fire a `PolarisEvent` here?
(I don't really know what an IRC server is supposed to do with these scan or
commit metrics 😅)
--
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]
