sanpwc commented on code in PR #6350:
URL: https://github.com/apache/ignite-3/pull/6350#discussion_r2248641664
##########
modules/metrics/src/main/java/org/apache/ignite/internal/metrics/MetricManagerImpl.java:
##########
@@ -121,7 +128,10 @@ public void start(Map<String, MetricExporter>
availableExporters) {
MetricView conf = metricConfiguration.value();
- for (ExporterView exporter : conf.exporters()) {
+ List<ExporterView> exporters =
StreamSupport.stream(conf.exporters().spliterator(), false).collect(toList());
Review Comment:
Is `StreamSupport.stream(conf.exporters().spliterator(), false)` just a way
to retrieve values from namedListView? If true, why not to use
`List<ExporterView> exporters = conf.exporters().stream().collect(toList());` ?
##########
modules/metrics-exporter-otlp/src/integrationTest/java/org/apache/ignite/internal/metrics/exporters/ItOtlpMetricsTest.java:
##########
@@ -95,7 +93,8 @@ void otlpMetricsGetCorrectlyReported() {
cluster.runningNodes().forEach(node -> {
MetricManager metricManager =
unwrapIgniteImpl(node).metricManager();
- assertThat(metricManager.enabledExporters(),
contains(instanceOf(OtlpPushMetricExporter.class)));
+ boolean contains =
metricManager.enabledExporters().stream().anyMatch(e ->
e.getClass().equals(OtlpPushMetricExporter.class));
Review Comment:
Since we already widely use assert I'd rather use
```
import static org.assertj.core.api.Assertions.assertThat;
...
assertThat(metricManager.enabledExporters()).hasAtLeastOneElementOfType(OtlpPushMetricExporter.class);
```
or similar. This looks more concise.
##########
modules/metrics/src/main/java/org/apache/ignite/internal/metrics/MetricManagerImpl.java:
##########
@@ -247,6 +257,43 @@ public Collection<MetricExporter> enabledExporters() {
return inBusyLock(busyLock, enabledMetricExporters::values);
}
+ private static List<ExporterView> defaultExporters(List<? extends
ExporterView> configuredExporters) {
+ if
(configuredExporters.stream().map(ExporterView::exporterName).anyMatch(n ->
n.equals(LogPushExporter.EXPORTER_NAME))) {
Review Comment:
Could you please elaborate this. Why do we believe that there are no default
exporters if there's "logPush" in the configuredExporters list?
##########
modules/metrics/src/main/java/org/apache/ignite/internal/metrics/exporters/configuration/LogPushExporterConfigurationSchema.java:
##########
@@ -38,8 +38,13 @@ public class LogPushExporterConfigurationSchema extends
ExporterConfigurationSch
/**
* List of enabled metric sources. If not empty, metric sources that are
not enumerated will be not printed.
Review Comment:
will be not printed -> will not be printed.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
