OneSizeFitsQuorum commented on code in PR #11410:
URL: https://github.com/apache/iotdb/pull/11410#discussion_r1375763862
##########
iotdb-core/metrics/core/src/main/java/org/apache/iotdb/metrics/core/type/IoTDBHistogram.java:
##########
@@ -17,19 +17,50 @@
* under the License.
*/
-package org.apache.iotdb.metrics.micrometer.type;
+package org.apache.iotdb.metrics.core.type;
+import
org.apache.iotdb.metrics.core.type.IoTDBHistogramMBean.AbstractJmxHistogramBean;
import org.apache.iotdb.metrics.type.Histogram;
-public class MicrometerHistogram implements Histogram {
+public class IoTDBHistogram extends AbstractJmxHistogramBean
+ implements Histogram, IoTDBHistogramMBean {
io.micrometer.core.instrument.DistributionSummary distributionSummary;
- public MicrometerHistogram(
- io.micrometer.core.instrument.DistributionSummary distributionSummary) {
+ public IoTDBHistogram(io.micrometer.core.instrument.DistributionSummary
distributionSummary) {
this.distributionSummary = distributionSummary;
}
+ @Override
+ public long getCount() {
+ return this.count();
+ }
+
+ @Override
+ public double getMax() {
Review Comment:
It looks like calling multiple getSize and getMax functions like this will
result in multiple snapshots.Is this an optimization necessary?
If only jmxreporter was going to use these functions, maybe we could just
write a todo instead of optimizing for now
##########
iotdb-core/metrics/core/src/main/java/org/apache/iotdb/metrics/core/IoTDBMetricManager.java:
##########
@@ -0,0 +1,166 @@
+/*
+ * 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.iotdb.metrics.core;
+
+import org.apache.iotdb.metrics.AbstractMetricManager;
+import org.apache.iotdb.metrics.core.type.IoTDBAutoGauge;
+import org.apache.iotdb.metrics.core.type.IoTDBCounter;
+import org.apache.iotdb.metrics.core.type.IoTDBGauge;
+import org.apache.iotdb.metrics.core.type.IoTDBHistogram;
+import org.apache.iotdb.metrics.core.type.IoTDBRate;
+import org.apache.iotdb.metrics.core.type.IoTDBTimer;
+import org.apache.iotdb.metrics.type.AutoGauge;
+import org.apache.iotdb.metrics.type.Counter;
+import org.apache.iotdb.metrics.type.Gauge;
+import org.apache.iotdb.metrics.type.Histogram;
+import org.apache.iotdb.metrics.type.Rate;
+import org.apache.iotdb.metrics.type.Timer;
+import org.apache.iotdb.metrics.utils.MetricInfo;
+import org.apache.iotdb.metrics.utils.MetricType;
+
+import io.micrometer.core.instrument.Clock;
+import io.micrometer.core.instrument.cumulative.CumulativeDistributionSummary;
+import io.micrometer.core.instrument.cumulative.CumulativeTimer;
+import io.micrometer.core.instrument.distribution.DistributionStatisticConfig;
+import io.micrometer.core.instrument.distribution.pause.NoPauseDetector;
+import io.micrometer.core.instrument.distribution.pause.PauseDetector;
+import io.micrometer.core.instrument.simple.SimpleConfig;
+
+import java.util.Objects;
+import java.util.concurrent.TimeUnit;
+import java.util.function.ToDoubleFunction;
+
+public class IoTDBMetricManager extends AbstractMetricManager {
+
+ /** The clock which is used in Metric system */
+ private final Clock clock = Clock.SYSTEM;
+ /** The Micrometer framework config */
+ private final SimpleConfig config = SimpleConfig.DEFAULT;
+ /** The default histogram config which is used to create IoTDBTimer and
IoTDBHistogram */
+ private final DistributionStatisticConfig defaultHistogramConfig =
+ DistributionStatisticConfig.builder()
+ .expiry(config.step())
+ .build()
+ .merge(DistributionStatisticConfig.DEFAULT);
+
+ private IoTDBMetricManager() {
+ // empty body
+ }
+
+ @Override
+ public Counter createCounter() {
+ return new IoTDBCounter();
+ }
+
+ public <T> AutoGauge createAutoGauge(T obj, ToDoubleFunction<T> mapper) {
+ return new IoTDBAutoGauge<>(obj, mapper);
+ }
+
+ @Override
+ public Gauge createGauge() {
+ return new IoTDBGauge();
+ }
+
+ @Override
+ public Histogram createHistogram(MetricInfo metricInfo) {
+ // merge default config with IoTDB's config
+ DistributionStatisticConfig distributionStatisticConfig =
+ DistributionStatisticConfig.builder()
+ .percentiles(0.5, 0.99)
+ .build()
+ .merge(defaultHistogramConfig);
+
+ // set expiry
+ DistributionStatisticConfig merged =
Review Comment:
It looks like config is read-only, and both timer and histogram are using
the same `merged`, so can we just make it a shared class variable?
##########
iotdb-core/metrics/interface/src/main/java/org/apache/iotdb/metrics/AbstractMetricManager.java:
##########
@@ -51,12 +52,44 @@ public abstract class AbstractMetricManager {
protected Map<String, MetricInfo.MetaInfo> nameToMetaInfo;
/** The map from metricInfo to metric. */
protected Map<MetricInfo, IMetric> metrics;
+ /** The bind IoTDBJmxReporter */
+ protected JmxReporter bindJmxReporter = null;
protected AbstractMetricManager() {
nameToMetaInfo = new ConcurrentHashMap<>();
metrics = new ConcurrentHashMap<>();
}
+ /**
+ * Notify IoTDB JmxReporter to register metric in JMX
+ *
+ * @param metric the created metric
+ * @param metricInfo the created metric info
+ */
+ private void notifyReporterOnAdd(IMetric metric, MetricInfo metricInfo) {
+ // if the reporter type is not JMX
+ if (bindJmxReporter == null) {
+ return;
+ }
+ // register the new metric
+ bindJmxReporter.registerMetric(metric, metricInfo);
+ }
+
+ /**
+ * Notify IoTDB JmxReporter to remove metric in JMX
+ *
+ * @param metric the removed metric
+ * @param metricInfo the removed metric info
+ */
+ private void notifyReporterOnRemove(IMetric metric, MetricInfo metricInfo) {
+ // if the reporter type is not JMX
+ if (bindJmxReporter == null) {
+ return;
+ }
+ // register the new metric
+ bindJmxReporter.unregisterMetric(metric, metricInfo);
Review Comment:
same
##########
iotdb-core/metrics/interface/src/main/java/org/apache/iotdb/metrics/AbstractMetricService.java:
##########
@@ -69,6 +64,10 @@ public abstract class AbstractMetricService {
/** The list of metric sets. */
protected Set<IMetricSet> metricSets = new HashSet<>();
+ // /** The name of default metric manager */
Review Comment:
do not comment fields
##########
iotdb-core/metrics/interface/src/main/java/org/apache/iotdb/metrics/AbstractMetricManager.java:
##########
@@ -51,12 +52,44 @@ public abstract class AbstractMetricManager {
protected Map<String, MetricInfo.MetaInfo> nameToMetaInfo;
/** The map from metricInfo to metric. */
protected Map<MetricInfo, IMetric> metrics;
+ /** The bind IoTDBJmxReporter */
+ protected JmxReporter bindJmxReporter = null;
protected AbstractMetricManager() {
nameToMetaInfo = new ConcurrentHashMap<>();
metrics = new ConcurrentHashMap<>();
}
+ /**
+ * Notify IoTDB JmxReporter to register metric in JMX
+ *
+ * @param metric the created metric
+ * @param metricInfo the created metric info
+ */
+ private void notifyReporterOnAdd(IMetric metric, MetricInfo metricInfo) {
+ // if the reporter type is not JMX
+ if (bindJmxReporter == null) {
Review Comment:
could use `optional.ofNullable(bindJmxReporter).map(x ->
bindJmxReporter.registerMetric(metric, metricInfo);`
##########
iotdb-core/metrics/core/src/main/java/org/apache/iotdb/metrics/core/uitls/IoTDBMetricObjNameFactory.java:
##########
@@ -0,0 +1,106 @@
+/*
+ * 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.iotdb.metrics.core.uitls;
+
+import org.apache.iotdb.metrics.core.reporter.IoTDBJmxReporter;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.management.MalformedObjectNameException;
+import javax.management.ObjectName;
+
+import java.util.Hashtable;
+
+public class IoTDBMetricObjNameFactory implements ObjectNameFactory {
+ private static final char[] QUOTABLE_CHARS = new char[] {',', '=', ':', '"'};
+ private static final Logger LOGGER =
LoggerFactory.getLogger(IoTDBJmxReporter.class);
Review Comment:
IoTDBMetricObjNameFactory.class
##########
iotdb-core/metrics/interface/src/main/java/org/apache/iotdb/metrics/impl/DoNothingGauge.java:
##########
@@ -41,4 +43,9 @@ public void decr(long value) {
public void set(long value) {
// do nothing
}
+
+ @Override
+ public void setObjectName(ObjectName objectName) {
Review Comment:
I wondered if it was necessary to implement a base class or interface with a
default implementation for this function? This way we don't have to write this
function in so many classes
##########
iotdb-core/metrics/interface/src/main/java/org/apache/iotdb/metrics/reporter/prometheus/PrometheusReporter.java:
##########
@@ -136,15 +136,11 @@ private String scrape() {
prometheusTextWriter.writeHelp(name);
prometheusTextWriter.writeType(name,
metricInfo.getMetaInfo().getType());
Rate rate = (Rate) metric;
- prometheusTextWriter.writeSample(name, metricInfo.getTags(),
rate.getCount());
+ prometheusTextWriter.writeSample(name, metricInfo.getTags(),
rate.count());
prometheusTextWriter.writeSample(
- name, addTags(metricInfo.getTags(), "rate", "m1"),
rate.getOneMinuteRate());
+ name, addTags(metricInfo.getTags(), "rate", "m1"),
rate.oneMinuteRate());
Review Comment:
if we use getOneMinuteRate here, then we can remove function `oneMinuteRate`?
##########
iotdb-core/metrics/core/src/main/java/org/apache/iotdb/metrics/core/type/IoTDBTimer.java:
##########
@@ -39,11 +40,46 @@ public void update(long duration, TimeUnit unit) {
@Override
public HistogramSnapshot takeSnapshot() {
- return new MicrometerTimerHistogramSnapshot(timer);
+ return new IoTDBTimerHistogramSnapshot(timer);
}
@Override
public long getCount() {
+ return this.count();
+ }
+
+ @Override
+ public double getSum() {
+ return this.takeSnapshot().getSum();
+ }
+
+ @Override
+ public double getMax() {
+ return this.takeSnapshot().getMax();
+ }
+
+ @Override
+ public double getMean() {
+ return this.takeSnapshot().getMean();
Review Comment:
same as above
##########
iotdb-core/metrics/core/src/main/java/org/apache/iotdb/metrics/core/uitls/IoTDBCachedGauge.java:
##########
@@ -0,0 +1,114 @@
+/*
+ * 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.iotdb.metrics.core.uitls;
+
+import
org.apache.iotdb.metrics.core.uitls.IoTDBCachedGaugeMBean.AbstractJmxCachedGaugeMBean;
+import org.apache.iotdb.metrics.type.AutoGauge;
+
+import com.codahale.metrics.Clock;
+
+import java.lang.ref.WeakReference;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.ToDoubleFunction;
+
+/**
+ * Gauges with cache, which have better performance in some read-intensive and
calculation-intensive
+ * scenarios. Can be served as an additional option.
+ */
+public class IoTDBCachedGauge<T> extends AbstractJmxCachedGaugeMBean
+ implements AutoGauge, IoTDBCachedGaugeMBean {
+ /** The timer of metric system */
+ private final Clock clock;
+ /** The time to reload cache */
+ private final AtomicLong reloadAt;
+ /** The timeout duration */
+ private final long timeoutNS;
+ /** The cache's value */
+ private final AtomicReference<Double> value;
+ /** The reference object of gauge */
+ private final WeakReference<T> refObj;
+ /** The calculate function of gauge */
+ private final ToDoubleFunction<T> mapper;
+
+ protected IoTDBCachedGauge(
+ WeakReference<T> refObj, ToDoubleFunction<T> mapper, long timeout,
TimeUnit timeoutUnit) {
+ this(Clock.defaultClock(), refObj, mapper, timeout, timeoutUnit);
+ }
+
+ protected IoTDBCachedGauge(
+ Clock clock,
+ WeakReference<T> refObj,
+ ToDoubleFunction<T> mapper,
+ long timeout,
+ TimeUnit timeoutUnit) {
+ this.clock = clock;
+ this.refObj = refObj;
+ this.mapper = mapper;
+ this.reloadAt = new AtomicLong(clock.getTick());
+ this.timeoutNS = timeoutUnit.toNanos(timeout);
+ this.value = new AtomicReference<>();
+ }
+
+ @Override
+ public Number getValue() {
+ return this.value();
+ }
+
+ @Override
+ public double value() {
+ return getGaugeValue();
+ }
+
+ private double loadValue() {
+ if (refObj.get() == null) {
+ return 0d;
+ }
+ return mapper.applyAsDouble(refObj.get());
+ }
+
+ public double getGaugeValue() {
+ Double currentValue = this.value.get();
+ // if cache expires or is not loaded, we update the cache with calculate
function
+ if (shouldLoad() || currentValue == null) {
+ double newValue = loadValue();
+ if (!this.value.compareAndSet(currentValue, newValue)) {
+ return this.value.get();
+ }
+ return newValue;
+ }
+ // else we directly return the cache's value
+ return currentValue;
Review Comment:
The code here is a bit strange, I suggest adding a comment to say that the
code is borrowed from micrometer and should be bug free
##########
iotdb-core/metrics/core/src/main/java/org/apache/iotdb/metrics/core/type/IoTDBRate.java:
##########
@@ -33,49 +34,50 @@
* <p>Now, we only record a gauge for the rate record in micrometer, and we
use dropwizard meter to
* calculate the meter.
*/
-public class MicrometerRate implements Rate {
- AtomicLong atomicLong;
+public class IoTDBRate extends AbstractJmxRateBean implements Rate,
IoTDBRateMBean {
Meter meter;
- public MicrometerRate(AtomicLong atomicLong) {
- this.atomicLong = atomicLong;
- this.meter = new Meter();
+ public IoTDBRate() {
+ this.meter = new Meter(new IoTDBMovingAverage(), Clock.defaultClock());
}
@Override
public long getCount() {
- return meter.getCount();
+ return this.count();
+ }
+
+ @Override
+ public double getMeanRate() {
+ return this.meanRate();
}
@Override
public double getOneMinuteRate() {
- return meter.getOneMinuteRate();
+ return this.oneMinuteRate();
}
@Override
- public double getMeanRate() {
- return meter.getMeanRate();
+ public long count() {
+ return meter.getCount();
}
@Override
- public double getFiveMinuteRate() {
- return meter.getFiveMinuteRate();
+ public double oneMinuteRate() {
+ return meter.getOneMinuteRate();
}
Review Comment:
Is it possible to just expose a public method if the meaning is the same but
the name is different
--
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]
