[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #6398: Meter system enhancement

[GitBox <gi...@apache.org>]

kezhenxu94 commented on a change in pull request #6398:
URL: https://github.com/apache/skywalking/pull/6398#discussion_r578947706



##########
File path: CHANGES.md
##########
@@ -20,6 +20,7 @@ Release Notes.
 * Update the `timestamp` field type for `LogQuery`.
 * Support Zabbix protocol to receive agent metrics.
 * Update the Apdex metric combine calculator.
+* Enhancement `MeterSystem` allow creating metrics with same `metricName` / `function` / `scope`.

Review comment:
       ```suggestion
   * Enhancement `MeterSystem` to allow creating metrics with same `metricName` / `function` / `scope`.
   ```

##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/meter/MeterSystem.java
##########
@@ -187,6 +187,22 @@ public MeterSystem(final ModuleManager manager) {
             throw new IllegalArgumentException("Function " + functionName + " can't be found by javaassist.");
         }
         final String className = formatName(metricsName);
+
+        /**
+         * Check is already define the metric
+         */
+        try {
+            CtClass alreadyMetric = classPool.get(METER_CLASS_PACKAGE + className);
+            if (alreadyMetric.getSuperclass() != parentClass || type != meterPrototypes.get(metricsName).getScopeType()) {
+                throw new IllegalArgumentException("Already define metric " + metricsName
+                                                       + ", but calculate function or scope type is not same.");

Review comment:
       ```suggestion
                   throw new IllegalArgumentException("Already defined metrics class " + metricsName
                                                          + ", but calculate function or scope type is different.");
   ```

##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/meter/MeterSystem.java
##########
@@ -187,6 +187,22 @@ public MeterSystem(final ModuleManager manager) {
             throw new IllegalArgumentException("Function " + functionName + " can't be found by javaassist.");
         }
         final String className = formatName(metricsName);
+
+        /**
+         * Check is already define the metric
+         */
+        try {
+            CtClass alreadyMetric = classPool.get(METER_CLASS_PACKAGE + className);
+            if (alreadyMetric.getSuperclass() != parentClass || type != meterPrototypes.get(metricsName).getScopeType()) {
+                throw new IllegalArgumentException("Already define metric " + metricsName
+                                                       + ", but calculate function or scope type is not same.");
+            }
+            log.debug("Metric {} is already define, so skip the metric creation.", metricsName);
+            return true;
+        } catch (NotFoundException e) {
+            // Ignore not found
+        }

Review comment:
       ```suggestion
           } catch (NotFoundException ignored) {
           }
   ```

##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/meter/MeterSystem.java
##########
@@ -187,6 +187,22 @@ public MeterSystem(final ModuleManager manager) {
             throw new IllegalArgumentException("Function " + functionName + " can't be found by javaassist.");
         }
         final String className = formatName(metricsName);
+
+        /**
+         * Check is already define the metric

Review comment:
       ```suggestion
            * Check whether the metrics class is already defined or not
   ```




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org