adutra commented on code in PR #2887:
URL: https://github.com/apache/polaris/pull/2887#discussion_r2469505744
##########
runtime/service/src/test/java/org/apache/polaris/service/reporting/DefaultMetricsReporterTest.java:
##########
@@ -0,0 +1,42 @@
+package org.apache.polaris.service.reporting;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+
+import com.google.common.collect.ImmutableMap;
+import io.quarkus.test.junit.QuarkusTest;
+import io.quarkus.test.junit.TestProfile;
+import io.smallrye.mutiny.tuples.Functions.TriConsumer;
+import java.util.Map;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.metrics.MetricsReport;
+import org.apache.polaris.service.catalog.Profiles;
+import org.junit.jupiter.api.Test;
+
+@QuarkusTest
+@TestProfile(DefaultMetricsReporterTest.Profile.class)
Review Comment:
This is just a unit test, you don't need to start the whole application.
```suggestion
```
##########
runtime/service/src/main/java/org/apache/polaris/service/reporting/DefaultMetricsReporter.java:
##########
@@ -0,0 +1,35 @@
+package org.apache.polaris.service.reporting;
+
+import com.google.common.annotations.VisibleForTesting;
+import io.smallrye.common.annotation.Identifier;
+import io.smallrye.mutiny.tuples.Functions.TriConsumer;
Review Comment:
```suggestion
import org.apache.commons.lang3.function.TriConsumer;
```
There is nothing wrong per se with Mutiny's `TriConsumer`, but the one from
Commons Lang is more general-purpose.
##########
runtime/defaults/src/main/resources/application.properties:
##########
@@ -209,6 +209,11 @@ polaris.oidc.principal-roles-mapper.type=default
# Polaris Credential Manager Config
polaris.credential-manager.type=default
+# Configuration for the behaviour of the metrics endpoint
+polaris.iceberg-metrics.reporting.type=default
+# Set to INFO if you want to see iceberg metric reports printed to console
+quarkus.log.category."polaris.service.reporting".level=WARN
Review Comment:
```suggestion
quarkus.log.category."org.apache.polaris.service.reporting".level=OFF
```
##########
runtime/service/src/main/java/org/apache/polaris/service/reporting/DefaultMetricsReporter.java:
##########
@@ -0,0 +1,35 @@
+package org.apache.polaris.service.reporting;
+
+import com.google.common.annotations.VisibleForTesting;
+import io.smallrye.common.annotation.Identifier;
+import io.smallrye.mutiny.tuples.Functions.TriConsumer;
+import jakarta.enterprise.context.RequestScoped;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.metrics.MetricsReport;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@RequestScoped
+@Identifier("default")
+public class DefaultMetricsReporter implements PolarisMetricsReporter {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(DefaultMetricsReporter.class);
+
+ private final TriConsumer<String, TableIdentifier, MetricsReport>
reportConsumer;
+
+ public DefaultMetricsReporter() {
+ this(
+ (warehouse, table, metricsReport) ->
+ LOGGER.info("{}.{}: {}", warehouse, table, metricsReport));
+ }
+
+ @VisibleForTesting
+ public DefaultMetricsReporter(
Review Comment:
```suggestion
DefaultMetricsReporter(
```
##########
runtime/service/src/test/java/org/apache/polaris/service/reporting/DefaultMetricsReporterTest.java:
##########
@@ -0,0 +1,42 @@
+package org.apache.polaris.service.reporting;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+
+import com.google.common.collect.ImmutableMap;
+import io.quarkus.test.junit.QuarkusTest;
+import io.quarkus.test.junit.TestProfile;
+import io.smallrye.mutiny.tuples.Functions.TriConsumer;
+import java.util.Map;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.metrics.MetricsReport;
+import org.apache.polaris.service.catalog.Profiles;
+import org.junit.jupiter.api.Test;
+
+@QuarkusTest
+@TestProfile(DefaultMetricsReporterTest.Profile.class)
+public class DefaultMetricsReporterTest {
+
+ @Test
+ void testLogging() {
+ TriConsumer<String, TableIdentifier, MetricsReport> mockConsumer =
mock(TriConsumer.class);
+ DefaultMetricsReporter reporter = new DefaultMetricsReporter(mockConsumer);
+ String warehouse = "testWarehouse";
+ TableIdentifier table = TableIdentifier.of("testNamespace", "testTable");
+ MetricsReport metricsReport = mock(MetricsReport.class);
+
+ reporter.reportMetric(warehouse, table, metricsReport);
+
+ verify(mockConsumer).accept(warehouse, table, metricsReport);
+ }
+
+ public static class Profile extends Profiles.DefaultProfile {
+ @Override
+ public Map<String, String> getConfigOverrides() {
+ return ImmutableMap.<String, String>builder()
+ .putAll(super.getConfigOverrides())
+ .put("quarkus.log.category.\"polaris.service.reporting\".level",
"INFO")
+ .build();
+ }
+ }
Review Comment:
```suggestion
```
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogIntegrationBase.java:
##########
@@ -882,6 +882,27 @@ public void testCreateAndLoadTableWithReturnedEtag() {
}
}
+ @Test
+ public void testSendMetricsReport() {
+ Map<String, String> payload =
+ ImmutableMap.<String, String>builder()
+ .put("reportType", "SCAN_REPORT")
+ .put(
+ "report",
+ "{\"scanId\":\""
+ + UUID.randomUUID()
+ + "\",\"timestamp\":"
+ + System.currentTimeMillis()
+ +
",\"filesScanned\":1,\"rowsScanned\":100,\"metrics\":{\"bytesRead\":1024}}")
+ .build();
+ Invocation.Builder metricEndpoint =
+ catalogApi.request(
+ "v1/{cat}/namespaces/ns1/tables/tbl1/metrics", Map.of("cat",
currentCatalogName));
+ try (Response response = metricEndpoint.post(Entity.json(payload))) {
+ assertThat(response).returns(Response.Status.NO_CONTENT.getStatusCode(),
Response::getStatus);
+ }
Review Comment:
```suggestion
ScanReport scanReport =
ImmutableScanReport.builder()
.tableName("tbl1")
.schemaId(4)
.addProjectedFieldIds(1, 2, 3)
.addProjectedFieldNames("c1", "c2", "c3")
.snapshotId(23L)
.filter(Expressions.alwaysTrue())
.scanMetrics(ScanMetricsResult.fromScanMetrics(ScanMetrics.noop()))
.build();
Invocation.Builder metricEndpoint =
catalogApi.request(
"v1/{cat}/namespaces/ns1/tables/tbl1/metrics", Map.of("cat",
currentCatalogName));
try (Response response =
metricEndpoint.post(Entity.json(ReportMetricsRequest.of(scanReport)))) {
assertThat(response).returns(Response.Status.NO_CONTENT.getStatusCode(),
Response::getStatus);
}
```
--
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]
