You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/08/24 03:00:46 UTC
[GitHub] [ignite-3] nva commented on a diff in pull request #1027: IGNITE-17444 Introduce MetricExporter interface and service loading.
nva commented on code in PR #1027:
URL: https://github.com/apache/ignite-3/pull/1027#discussion_r953271373
##########
modules/metrics/src/integrationTest/java/org/apache/ignite/internal/metrics/exporters/MetricExportersLoadingTest.java:
##########
@@ -0,0 +1,78 @@
+/*
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.metrics.exporters;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.io.ByteArrayOutputStream;
+import java.io.OutputStream;
+import java.util.concurrent.locks.LockSupport;
+import org.apache.ignite.internal.metrics.MetricManager;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Integration test for metrics' exporters loading.
+ */
+public class MetricExportersLoadingTest {
+
Review Comment:
https://google.github.io/styleguide/javaguide.html#s4.1.2-blocks-k-r-style
```suggestion
```
##########
modules/metrics/src/integrationTest/java/org/apache/ignite/internal/metrics/exporters/MetricExportersLoadingTest.java:
##########
@@ -0,0 +1,78 @@
+/*
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.metrics.exporters;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.io.ByteArrayOutputStream;
+import java.io.OutputStream;
+import java.util.concurrent.locks.LockSupport;
+import org.apache.ignite.internal.metrics.MetricManager;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Integration test for metrics' exporters loading.
+ */
+public class MetricExportersLoadingTest {
+
+ @Test
+ public void test() throws Exception {
+ MetricManager metricManager = new MetricManager();
+
+ TestMetricsSource src = new TestMetricsSource("TestMetricsSource");
+
+ metricManager.registerSource(src);
+
+ metricManager.enable(src.name());
+
+ OutputStream pullOutputStream = new ByteArrayOutputStream();
+ TestPullMetricExporter.setOutputStream(pullOutputStream);
+
+ OutputStream pushOutputStream = new ByteArrayOutputStream();
+ TestPushMetricExporter.setOutputStream(pushOutputStream);
+
+ assertEquals(0, pullOutputStream.toString().length());
+
+ assertEquals(0, pushOutputStream.toString().length());
+
+ metricManager.start();
+
+ src.inc();
+
+ waitForOutput(pushOutputStream, "TestMetricsSource:\nmetric:1");
+ assertTrue(pushOutputStream.toString().contains("TestMetricsSource:\nmetric:1"));
+
+ TestPullMetricExporter.requestMetrics();
+
+ waitForOutput(pullOutputStream, "TestMetricsSource:\nmetric:1");
+ assertTrue(pullOutputStream.toString().contains("TestMetricsSource:\nmetric:1"));
+
+ metricManager.stop();
+
+ pushOutputStream.close();
+ pullOutputStream.close();
Review Comment:
try-with-resources?
##########
modules/metrics/src/main/java/org/apache/ignite/internal/metrics/MetricProvider.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.metrics;
+
+import java.util.Map;
+
+/**
+ * Read-only metrics registry.
+ */
+public class MetricProvider {
+
Review Comment:
```suggestion
```
##########
modules/metrics/src/integrationTest/java/org/apache/ignite/internal/metrics/exporters/TestPullMetricExporter.java:
##########
@@ -0,0 +1,100 @@
+/*
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.metrics.exporters;
+
+import java.io.IOException;
+import java.io.OutputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import org.apache.ignite.internal.metrics.Metric;
+import org.apache.ignite.internal.metrics.MetricSet;
+
+/**
+ * Simple pull exporter, which simulate the pull principe throw primitive wait/notify API
+ * instead of the complex TCP/IP etc. endpoints.
+ */
+public class TestPullMetricExporter extends BasicMetricExporter {
+
Review Comment:
```suggestion
```
##########
modules/metrics/src/integrationTest/java/org/apache/ignite/internal/metrics/exporters/TestPullMetricExporter.java:
##########
@@ -0,0 +1,100 @@
+/*
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.metrics.exporters;
+
+import java.io.IOException;
+import java.io.OutputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import org.apache.ignite.internal.metrics.Metric;
+import org.apache.ignite.internal.metrics.MetricSet;
+
+/**
+ * Simple pull exporter, which simulate the pull principe throw primitive wait/notify API
+ * instead of the complex TCP/IP etc. endpoints.
+ */
+public class TestPullMetricExporter extends BasicMetricExporter {
+
+ private static OutputStream outputStream;
+
+ private static final Object obj = new Object();
+
+ ExecutorService executorService = Executors.newSingleThreadExecutor();
+
+ public static void setOutputStream(OutputStream outputStream) {
+ TestPullMetricExporter.outputStream = outputStream;
+ }
+
+ /**
+ * Simulate metric request.
+ */
+ public static void requestMetrics() {
+ synchronized (obj) {
+ obj.notify();
+ }
+ }
+
+ @Override
+ public void start() {
+ executorService.execute(() -> {
+ while (true) {
+ waitForRequest();
+
+ var report = new StringBuilder();
+
+ for (MetricSet metricSet : metrics().values()) {
+ report.append(metricSet.name()).append(":\n");
+
+ for (Metric metric : metricSet) {
+ report.append(metric.name())
+ .append(":")
+ .append(metric.getValueAsString())
+ .append("\n");
+ }
+
+ report.append("\n");
+ }
+
+ try {
+ outputStream.write(report.toString().getBytes(StandardCharsets.UTF_8));
+
+ outputStream.flush();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+ });
+ }
+
+ @Override
+ public void stop() {
+ executorService.shutdown();
+ }
+
+ private void waitForRequest() {
+ synchronized (obj) {
+ try {
+ obj.wait();
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
Review Comment:
```suggestion
```
##########
modules/metrics/src/main/java/org/apache/ignite/internal/metrics/exporters/MetricExporter.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.metrics.exporters;
+
+import org.apache.ignite.internal.metrics.MetricProvider;
+
+/**
+ * Interface for metric exporters to external recipients.
+ * Exporters can be one of the two type: push and pull exporters.
+ *
+ * <p>Push exporters push metrics to the external endpoint periodically.
+ * Push exporters should implement {@link PushMetricExporter} according to its documentation.
+ *
+ * <p>Pull exporters is the endpoint by itself (HTTP, JMX and etc.), which response with the metric data for request.
+ * Pull exporters should extend {@link BasicMetricExporter}.
+ */
+public interface MetricExporter {
+
Review Comment:
```suggestion
```
##########
modules/metrics/src/integrationTest/java/org/apache/ignite/internal/metrics/exporters/MetricExportersLoadingTest.java:
##########
@@ -0,0 +1,78 @@
+/*
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.metrics.exporters;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.io.ByteArrayOutputStream;
+import java.io.OutputStream;
+import java.util.concurrent.locks.LockSupport;
+import org.apache.ignite.internal.metrics.MetricManager;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Integration test for metrics' exporters loading.
+ */
+public class MetricExportersLoadingTest {
+
+ @Test
+ public void test() throws Exception {
+ MetricManager metricManager = new MetricManager();
+
+ TestMetricsSource src = new TestMetricsSource("TestMetricsSource");
+
+ metricManager.registerSource(src);
+
+ metricManager.enable(src.name());
+
+ OutputStream pullOutputStream = new ByteArrayOutputStream();
+ TestPullMetricExporter.setOutputStream(pullOutputStream);
+
+ OutputStream pushOutputStream = new ByteArrayOutputStream();
+ TestPushMetricExporter.setOutputStream(pushOutputStream);
+
+ assertEquals(0, pullOutputStream.toString().length());
+
+ assertEquals(0, pushOutputStream.toString().length());
+
+ metricManager.start();
+
+ src.inc();
+
+ waitForOutput(pushOutputStream, "TestMetricsSource:\nmetric:1");
+ assertTrue(pushOutputStream.toString().contains("TestMetricsSource:\nmetric:1"));
+
+ TestPullMetricExporter.requestMetrics();
+
+ waitForOutput(pullOutputStream, "TestMetricsSource:\nmetric:1");
+ assertTrue(pullOutputStream.toString().contains("TestMetricsSource:\nmetric:1"));
+
+ metricManager.stop();
+
+ pushOutputStream.close();
+ pullOutputStream.close();
+ }
+
+ private void waitForOutput(OutputStream outputStream, String content) {
+ while (!outputStream.toString().contains(content)) {
+ LockSupport.parkNanos(100_000_000);
+ }
+ }
+
Review Comment:
```suggestion
```
##########
modules/metrics/src/integrationTest/java/org/apache/ignite/internal/metrics/exporters/TestPushMetricExporter.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.metrics.exporters;
+
+import java.io.IOException;
+import java.io.OutputStream;
+import java.nio.charset.StandardCharsets;
+import org.apache.ignite.internal.metrics.Metric;
+import org.apache.ignite.internal.metrics.MetricSet;
+
+/**
+ * Test push metrics exporter.
+ */
+public class TestPushMetricExporter extends PushMetricExporter {
+ private static OutputStream outputStream;
+
+ public TestPushMetricExporter() {
+ setPeriod(100);
+ }
+
+ public static void setOutputStream(OutputStream outputStream) {
+ TestPushMetricExporter.outputStream = outputStream;
+ }
+
+ @Override
+ public void report() {
+ var report = new StringBuilder();
+
+ for (MetricSet metricSet : metrics().values()) {
+ report.append(metricSet.name()).append(":\n");
+
+ for (Metric metric : metricSet) {
+ report.append(metric.name())
+ .append(":")
+ .append(metric.getValueAsString())
+ .append("\n");
+ }
+
+ report.append("\n");
+ }
+
+ write(report.toString());
+ }
+
+ private void write(String report) {
+
+ try {
+ outputStream.write(report.getBytes(StandardCharsets.UTF_8));
+
+ outputStream.flush();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+
Review Comment:
```suggestion
```
##########
modules/metrics/src/integrationTest/java/org/apache/ignite/internal/metrics/exporters/TestPushMetricExporter.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * 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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.metrics.exporters;
+
+import java.io.IOException;
+import java.io.OutputStream;
+import java.nio.charset.StandardCharsets;
+import org.apache.ignite.internal.metrics.Metric;
+import org.apache.ignite.internal.metrics.MetricSet;
+
+/**
+ * Test push metrics exporter.
+ */
+public class TestPushMetricExporter extends PushMetricExporter {
+ private static OutputStream outputStream;
+
+ public TestPushMetricExporter() {
+ setPeriod(100);
+ }
+
+ public static void setOutputStream(OutputStream outputStream) {
+ TestPushMetricExporter.outputStream = outputStream;
+ }
+
+ @Override
+ public void report() {
+ var report = new StringBuilder();
+
+ for (MetricSet metricSet : metrics().values()) {
+ report.append(metricSet.name()).append(":\n");
+
+ for (Metric metric : metricSet) {
+ report.append(metric.name())
+ .append(":")
+ .append(metric.getValueAsString())
+ .append("\n");
+ }
+
+ report.append("\n");
+ }
+
+ write(report.toString());
+ }
+
+ private void write(String report) {
+
Review Comment:
```suggestion
```
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org