denis-chudov commented on code in PR #1234: URL: https://github.com/apache/ignite-3/pull/1234#discussion_r1008916639
########## modules/metrics/src/main/java/org/apache/ignite/internal/metrics/exporters/jmx/JmxExporter.java: ########## @@ -0,0 +1,165 @@ +/* + * 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.jmx; + +import static org.apache.ignite.internal.util.IgniteUtils.makeMbeanName; + +import java.lang.management.ManagementFactory; +import java.util.ArrayList; +import java.util.List; +import javax.management.JMException; +import javax.management.MalformedObjectNameException; +import javax.management.ObjectName; +import org.apache.ignite.internal.logger.IgniteLogger; +import org.apache.ignite.internal.logger.Loggers; +import org.apache.ignite.internal.metrics.MetricProvider; +import org.apache.ignite.internal.metrics.MetricSet; +import org.apache.ignite.internal.metrics.exporters.BasicMetricExporter; +import org.apache.ignite.internal.metrics.exporters.configuration.JmxExporterView; + +/** + * Exporter for Ignite metrics to JMX API. + * For each enabled {@link org.apache.ignite.internal.metrics.MetricSource} exporter provides + * a separate MBean with corresponding attribute per source's metric. + */ +public class JmxExporter extends BasicMetricExporter<JmxExporterView> { + /** + * Exporter name. Must be the same for configuration and exporter itself. + */ + public static final String JMX_EXPORTER_NAME = "jmx"; + + /** + * Group attribute of {@link ObjectName} shared for all metric MBeans. + */ + private static final String JMX_METRIC_GROUP = "metrics"; + + /** + * Logger. + */ + private static IgniteLogger LOG = Loggers.forClass(JmxExporter.class); + + /** + * Current registered MBeans. + */ + private final List<ObjectName> mbeans = new ArrayList<>(); + + /** + * {@inheritDoc} + */ + @Override + public synchronized void start(MetricProvider metricsProvider, JmxExporterView configuration) { + super.start(metricsProvider, configuration); + + for (MetricSet metricSet : metricsProvider.metrics().get1().values()) { + register(metricSet); + } + } + + /** + * {@inheritDoc} + */ + @Override + public synchronized void stop() { + mbeans.forEach(this::unregBean); + + mbeans.clear(); + } + + /** + * {@inheritDoc} + */ + @Override + public String name() { + return JMX_EXPORTER_NAME; + } + + /** + * {@inheritDoc} + * + * <p>Register new MBean for received metric set. + */ + @Override + public synchronized void addMetricSet(MetricSet metricSet) { + register(metricSet); + } + + /** + * {@inheritDoc} + * + * <p>Unregister MBean for removed metric set. + */ + @Override + public synchronized void removeMetricSet(String metricSet) { + unregister(metricSet); + } + + /** + * Register new MBean per metric set. + * + * @param metricSet Metric set. + */ + private void register(MetricSet metricSet) { + try { + MetricSetMbean metricSetMbean = new MetricSetMbean(metricSet); + + ObjectName mbean = ManagementFactory.getPlatformMBeanServer() + .registerMBean( + metricSetMbean, + makeMbeanName(JMX_METRIC_GROUP, metricSet.name())) + .getObjectName(); + + mbeans.add(mbean); + } catch (JMException e) { + LOG.error("MBean for metric set " + metricSet.name() + " can't be created.", e); + } + } + + /** + * Unregister MBean for specific metric set. + * + * @param metricSetName Metric set name. + */ + private void unregister(String metricSetName) { + try { + ObjectName mbeanName = makeMbeanName(JMX_METRIC_GROUP, metricSetName); + + boolean rmv = mbeans.remove(mbeanName); + + if (rmv) { + unregBean(mbeanName); + } else { + LOG.warn("Tried to unregister the MBean for non-registered metric set " + metricSetName); + } + } catch (MalformedObjectNameException e) { + LOG.error("MBean for metric set " + metricSetName + " can't be unregistered.", e); + } + } + + /** + * Unregister MBean by its' name. Review Comment: typo? ########## modules/metrics/src/main/java/org/apache/ignite/internal/metrics/exporters/jmx/JmxExporter.java: ########## @@ -0,0 +1,165 @@ +/* + * 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.jmx; + +import static org.apache.ignite.internal.util.IgniteUtils.makeMbeanName; + +import java.lang.management.ManagementFactory; +import java.util.ArrayList; +import java.util.List; +import javax.management.JMException; +import javax.management.MalformedObjectNameException; +import javax.management.ObjectName; +import org.apache.ignite.internal.logger.IgniteLogger; +import org.apache.ignite.internal.logger.Loggers; +import org.apache.ignite.internal.metrics.MetricProvider; +import org.apache.ignite.internal.metrics.MetricSet; +import org.apache.ignite.internal.metrics.exporters.BasicMetricExporter; +import org.apache.ignite.internal.metrics.exporters.configuration.JmxExporterView; + +/** + * Exporter for Ignite metrics to JMX API. + * For each enabled {@link org.apache.ignite.internal.metrics.MetricSource} exporter provides + * a separate MBean with corresponding attribute per source's metric. + */ +public class JmxExporter extends BasicMetricExporter<JmxExporterView> { + /** + * Exporter name. Must be the same for configuration and exporter itself. + */ + public static final String JMX_EXPORTER_NAME = "jmx"; + + /** + * Group attribute of {@link ObjectName} shared for all metric MBeans. + */ + private static final String JMX_METRIC_GROUP = "metrics"; + + /** + * Logger. + */ + private static IgniteLogger LOG = Loggers.forClass(JmxExporter.class); + + /** + * Current registered MBeans. + */ + private final List<ObjectName> mbeans = new ArrayList<>(); Review Comment: looks like nothing protects us from attempts of concurrent modification of this collection, are you sure that ArrayList is suitable here? ########## modules/metrics/src/main/java/org/apache/ignite/internal/metrics/MetricManager.java: ########## @@ -178,15 +192,19 @@ public static Map<String, MetricExporter> loadExporters() { */ public void disable(MetricSource src) { registry.disable(src); + + enabledMetricExporters.values().forEach(e -> e.removeMetricSet(src.name())); } /** - * Disable metric source by name. See {@link MetricRegistry#disable(String)}. + * Disable metric set by name. See {@link MetricRegistry#disable(String)}. * - * @param srcName Source name. + * @param metricSetName Metric set name. */ - public void disable(final String srcName) { - registry.disable(srcName); + public void disable(final String metricSetName) { Review Comment: I don't completely understand this change. It refers to `MetricRegistry#disable(String)` but it disables metric source by its name. Also, it brings inconsistency to API, as `MetricManager#enable(String)` enables metric source, not metric set. Finally, only metric source can be enabled/disabled. Could you explain? -- 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]
