adutra commented on code in PR #2887:
URL: https://github.com/apache/polaris/pull/2887#discussion_r2465297845
##########
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:
Another way to test logger is to modify your reporter implementation like
below:
```java
@RequestScoped
@Identifier("logging")
public class LoggingMetricsReporter implements PolarisMetricsReporter {
private static final Logger LOGGER =
LoggerFactory.getLogger(LoggingMetricsReporter.class);
// A consumer that can be mocked in tests; production code uses the
default consumer
// which logs to the slf4j logger.
private final TriConsumer<String, TableIdentifier, MetricsReport>
reportConsumer;
public LoggingMetricsReporter() {
this(
(warehouse, table, metricsReport) ->
LOGGER.info("{}.{}: {}", warehouse, table, metricsReport));
}
@VisibleForTesting
public LoggingMetricsReporter(
TriConsumer<String, TableIdentifier, MetricsReport> reportConsumer) {
this.reportConsumer = reportConsumer;
}
@Override
public void reportMetric(String warehouse, TableIdentifier table,
MetricsReport metricsReport) {
reportConsumer.accept(warehouse, table, metricsReport);
}
}
```
##########
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:
If that's not too much to ask, you could provide two tests:
1. A simple unit test that tests the reporter in isolation, testing that
messages are generated in the logs, using any of the techniques above.
2. A `@QuarkusTest` using a mock reporter (you can inject one in your test
using `@InjectMock`), testing that metrics received via the endpoint are
correctly passed to the reporter.
##########
runtime/defaults/src/main/resources/application.properties:
##########
@@ -209,6 +209,9 @@ 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.metrics.reporting.type=default
Review Comment:
Can we rename this to `polaris.iceberg-metrics.reporting.type`? I'm afraid
users will be confuse this feature with "standard" server metrics (which are
configured using `polaris.metrics.*`, cf line 153).
##########
runtime/service/src/main/java/org/apache/polaris/service/reporting/LoggingMetricsReporter.java:
##########
@@ -0,0 +1,44 @@
+package org.apache.polaris.service.reporting;
+
+import io.smallrye.common.annotation.Identifier;
+import jakarta.enterprise.context.RequestScoped;
+
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.metrics.CommitReport;
+import org.apache.iceberg.metrics.MetricsReport;
+import org.apache.iceberg.metrics.ScanReport;
+import org.apache.iceberg.rest.requests.ReportMetricsRequest;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@RequestScoped
+@Identifier("logging")
+public class LoggingMetricsReporter implements PolarisMetricsReporter {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(LoggingMetricsReporter.class);
+
+ @Override
+ public void reportMetric(
+ String warehouse, TableIdentifier table, MetricsReport metricsReport) {
+ StringBuilder reportString = new StringBuilder();
Review Comment:
1. I don't think you need this. The `toString()` methods for `ScanReport`
and `CommitReport` are generated by Java Immutables and contain all the
information already. Same for `TableIdentifier` the `toString()` method is well
defined.
2. The log pattern is malformed, there is one dangling placeholder.
Just this should be enough:
```java
LOGGER.info("{}.{}: {}", warehouse, table, metricsReport);
```
--
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]
