sk0x50 commented on code in PR #1098:
URL: https://github.com/apache/ignite-3/pull/1098#discussion_r979656580
##########
modules/metrics/src/integrationTest/java/org/apache/ignite/internal/metrics/exporters/TestPushMetricsExporterConfigurationSchema.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
Review Comment:
```suggestion
* contributor license agreements. See the NOTICE file distributed with
```
##########
modules/metrics/src/main/java/org/apache/ignite/internal/metrics/configuration/MetricConfigurationModule.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
Review Comment:
```suggestion
* the License. You may obtain a copy of the License at
```
##########
modules/metrics/src/main/java/org/apache/ignite/internal/metrics/configuration/MetricConfigurationModule.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
Review Comment:
```suggestion
* contributor license agreements. See the NOTICE file distributed with
```
##########
modules/metrics/src/main/java/org/apache/ignite/internal/metrics/exporters/configuration/ExporterConfigurationSchema.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
Review Comment:
```suggestion
* contributor license agreements. See the NOTICE file distributed with
```
##########
modules/metrics/src/main/java/org/apache/ignite/internal/metrics/exporters/configuration/JmxExporterConfigurationSchema.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
Review Comment:
```suggestion
* contributor license agreements. See the NOTICE file distributed with
```
##########
modules/metrics/src/main/java/org/apache/ignite/internal/metrics/MetricManager.java:
##########
@@ -159,4 +191,41 @@ public void disable(final String srcName) {
public IgniteBiTuple<Map<String, MetricSet>, Long> metricSnapshot() {
return registry.metricSnapshot();
}
+
+ private <T extends ExporterConfiguration> void checkAndStartExporter(
+ String exporterName,
+ T exporterConfiguration) {
+ MetricExporter<T> exporter = availableExporters.get(exporterName);
+
+ if (exporter != null) {
+ exporter.init(metricsProvider, exporterConfiguration);
+
+ exporter.start();
+
+ enabledMetricExporters.put(exporter.name(), exporter);
+ } else {
+ LOG.warn("Received configuration for unknown metric exporter with
the name '" + exporterName + "'");
+ }
+
+ }
+
+ private class ExporterConfigurationListeter implements
ConfigurationNamedListListener<ExporterView> {
Review Comment:
```suggestion
private class ExporterConfigurationListener implements
ConfigurationNamedListListener<ExporterView> {
```
##########
modules/metrics/pom.xml:
##########
@@ -38,6 +38,16 @@
<artifactId>ignite-core</artifactId>
</dependency>
+ <dependency>
Review Comment:
Please reflect these changes into the corresponding `build.gradle` file
##########
modules/metrics/src/main/java/org/apache/ignite/internal/metrics/exporters/PushMetricExporter.java:
##########
@@ -62,7 +57,7 @@ public void start() {
throw th;
}
- }, period, period, TimeUnit.MILLISECONDS);
+ }, period(), period(), TimeUnit.MILLISECONDS);
Review Comment:
1. `new NamedThreadFactory("metrics-exporter", log)` - it seems to me, it
would be nice to add a name of this exporter as well. Let's assume that a user
configure two different push exporters.
2. should `fut` and `schedular` fields be volatile?
3. is it possible, that `period()` returns a negative value? in that case
`scheduleWithFixedDelay` throws `IllegalArgumentException` and so
`PushMetricExporter#stop()` will throw `NullPointerException`
##########
modules/metrics/src/integrationTest/java/org/apache/ignite/internal/metrics/exporters/TestPushMetricsExporterConfigurationSchema.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
Review Comment:
```suggestion
* the License. You may obtain a copy of the License at
```
##########
modules/metrics/src/main/resources/META-INF/services/org.apache.ignite.internal.configuration.ConfigurationModule:
##########
@@ -0,0 +1,17 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
Review Comment:
```suggestion
# the License. You may obtain a copy of the License at
```
##########
modules/metrics/src/main/java/org/apache/ignite/internal/metrics/configuration/MetricConfigurationSchema.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
Review Comment:
```suggestion
* contributor license agreements. See the NOTICE file distributed with
```
##########
modules/metrics/src/integrationTest/java/org/apache/ignite/internal/metrics/exporters/TestPullMetricsExporterConfigurationSchema.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
Review Comment:
```suggestion
* the License. You may obtain a copy of the License at
```
##########
modules/metrics/src/main/java/org/apache/ignite/internal/metrics/configuration/MetricConfigurationSchema.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
Review Comment:
```suggestion
* the License. You may obtain a copy of the License at
```
##########
modules/metrics/src/integrationTest/java/org/apache/ignite/internal/metrics/exporters/TestPullMetricsExporterConfigurationSchema.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
Review Comment:
```suggestion
* contributor license agreements. See the NOTICE file distributed with
```
##########
modules/metrics/src/main/java/org/apache/ignite/internal/metrics/exporters/configuration/JmxExporterConfigurationSchema.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
Review Comment:
```suggestion
* the License. You may obtain a copy of the License at
```
##########
modules/metrics/src/main/java/org/apache/ignite/internal/metrics/exporters/configuration/ExporterConfigurationSchema.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
Review Comment:
```suggestion
* the License. You may obtain a copy of the License at
```
##########
modules/metrics/src/main/resources/META-INF/services/org.apache.ignite.internal.configuration.ConfigurationModule:
##########
@@ -0,0 +1,17 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
Review Comment:
```suggestion
# contributor license agreements. See the NOTICE file distributed with
```
##########
modules/metrics/src/main/java/org/apache/ignite/internal/metrics/MetricManager.java:
##########
@@ -33,38 +42,61 @@
* Metric manager.
*/
public class MetricManager implements IgniteComponent {
+
+ /** Logger. */
+ private static final IgniteLogger LOG =
Loggers.forClass(MetricManager.class);
+
/**
* Metric registry.
*/
private final MetricRegistry registry;
+ private final MetricProvider metricsProvider;
+
+ private final Map<String, MetricExporter> enabledMetricExporters = new
HashMap<>();
Review Comment:
`ConcurrentHashMap`?
##########
modules/metrics/src/integrationTest/java/org/apache/ignite/internal/metrics/exporters/MetricExportersLoadingTest.java:
##########
@@ -23,17 +23,37 @@
import java.io.ByteArrayOutputStream;
import java.io.OutputStream;
import java.util.concurrent.locks.LockSupport;
+import
org.apache.ignite.internal.configuration.testframework.ConfigurationExtension;
+import
org.apache.ignite.internal.configuration.testframework.InjectConfiguration;
import org.apache.ignite.internal.metrics.MetricManager;
+import org.apache.ignite.internal.metrics.configuration.MetricConfiguration;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
/**
* Integration test for metrics' exporters loading.
*/
+@ExtendWith({ConfigurationExtension.class})
public class MetricExportersLoadingTest {
+
+ @InjectConfiguration(
+ value = "mock.exporters = {"
+ + "testPull = {exporterName = testPull},"
+ + "testPush = {exporterName = testPush, period = 100},\n"
Review Comment:
It would be nice to use `System.lineSeparator()` instead of `\n`.
##########
modules/metrics/src/main/java/org/apache/ignite/internal/metrics/MetricManager.java:
##########
@@ -33,38 +42,61 @@
* Metric manager.
*/
public class MetricManager implements IgniteComponent {
+
+ /** Logger. */
+ private static final IgniteLogger LOG =
Loggers.forClass(MetricManager.class);
+
/**
* Metric registry.
*/
private final MetricRegistry registry;
+ private final MetricProvider metricsProvider;
+
+ private final Map<String, MetricExporter> enabledMetricExporters = new
HashMap<>();
+
/** Metrics' exporters. */
- private List<MetricExporter> metricExporters;
+ private Map<String, MetricExporter> availableExporters;
+
+ private MetricConfiguration metricConfiguration;
Review Comment:
In my understanding, at least this field should be `volatile`, because of we
cannot guarantee that `configure()` and `start()` methods are called in the
same thread or properly synchronized.
--
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]
