servicecomb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] WillemJiang closed pull request #562: [SCB-358] fix bug for monitor output id that register only name without any tags
Date Thu, 01 Mar 2018 11:02:46 GMT
WillemJiang closed pull request #562: [SCB-358] fix bug for monitor output id that register
only name without any tags
URL: https://github.com/apache/incubator-servicecomb-java-chassis/pull/562
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/foundations/foundation-common/src/test/java/org/apache/servicecomb/foundation/common/event/TestEventBus.java
b/foundations/foundation-common/src/test/java/org/apache/servicecomb/foundation/common/event/TestEventBus.java
index 5def0ad19..66a887232 100644
--- a/foundations/foundation-common/src/test/java/org/apache/servicecomb/foundation/common/event/TestEventBus.java
+++ b/foundations/foundation-common/src/test/java/org/apache/servicecomb/foundation/common/event/TestEventBus.java
@@ -23,26 +23,32 @@
 import java.util.concurrent.atomic.AtomicBoolean;
 
 import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Test;
 
 public class TestEventBus {
 
-  @Test
-  public void checkEventReceivedAndProcessed() {
-    AtomicBoolean eventReceived = new AtomicBoolean(false);
+  private AtomicBoolean eventReceived = new AtomicBoolean(false);
 
-    EventListener<String> listener = new EventListener<String>() {
-      @Override
-      public Class<String> getEventClass() {
-        return String.class;
-      }
+  private EventListener<String> listener = new EventListener<String>() {
+    @Override
+    public Class<String> getEventClass() {
+      return String.class;
+    }
 
-      @Override
-      public void process(String data) {
-        eventReceived.set(true);
-      }
-    };
+    @Override
+    public void process(String data) {
+      eventReceived.set(true);
+    }
+  };
+
+  @Before
+  public void reset() {
+    EventBus.getInstance().unregisterEventListener(listener);
+  }
 
+  @Test
+  public void checkEventReceivedAndProcessed() {
     EventBus.getInstance().registerEventListener(listener);
 
     EventBus.getInstance().triggerEvent("xxx");
@@ -52,21 +58,7 @@ public void process(String data) {
   }
 
   @Test
-  public void checkEventCanNotReceivedAfterUnregister() throws InterruptedException {
-    AtomicBoolean eventReceived = new AtomicBoolean(false);
-
-    EventListener<String> listener = new EventListener<String>() {
-      @Override
-      public Class<String> getEventClass() {
-        return String.class;
-      }
-
-      @Override
-      public void process(String data) {
-        eventReceived.set(true);
-      }
-    };
-
+  public void checkEventCanNotReceivedAfterUnregister() {
     EventBus.getInstance().registerEventListener(listener);
 
     EventBus.getInstance().triggerEvent("xxx");
@@ -78,7 +70,15 @@ public void process(String data) {
 
     EventBus.getInstance().unregisterEventListener(listener);
     EventBus.getInstance().triggerEvent("xxx");
-    Thread.sleep(1000);
+    Assert.assertFalse(eventReceived.get());
+  }
+
+  @Test
+  public void checkUnmatchTypeWillNotReceived() {
+    EventBus.getInstance().registerEventListener(listener);
+
+    //trigger a Integer type event object
+    EventBus.getInstance().triggerEvent(new Integer(1));
     Assert.assertFalse(eventReceived.get());
   }
 }
diff --git a/foundations/foundation-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/publish/Metric.java
b/foundations/foundation-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/publish/Metric.java
index 0c1ce873c..b894b5810 100644
--- a/foundations/foundation-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/publish/Metric.java
+++ b/foundations/foundation-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/publish/Metric.java
@@ -21,12 +21,13 @@
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
 
+import org.apache.servicecomb.foundation.common.exceptions.ServiceCombException;
 import org.apache.servicecomb.foundation.metrics.MetricsConst;
 
 public class Metric {
-  private final String name;
+  private String name;
 
-  private final Map<String, String> tags;
+  private Map<String, String> tags;
 
   private double value;
 
@@ -35,14 +36,41 @@ public String getName() {
   }
 
   public Metric(String id, double value) {
-    String[] nameAndTag = id.split("\\(");
-    this.tags = new HashMap<>();
-    String[] tagAnValues = nameAndTag[1].split("[=,)]");
-    for (int i = 0; i < tagAnValues.length; i += 2) {
-      this.tags.put(tagAnValues[i], tagAnValues[i + 1]);
+    if (validateMetricId(id)) {
+      this.tags = new HashMap<>();
+      this.value = value;
+      String[] nameAndTag = id.split("[()]");
+      if (nameAndTag.length == 1) {
+        processIdWithoutTags(id, nameAndTag[0]);
+      } else if (nameAndTag.length == 2) {
+        processIdHadTags(id, nameAndTag);
+      } else {
+        throw new ServiceCombException("bad format id " + id);
+      }
+    } else {
+      throw new ServiceCombException("bad format id " + id);
+    }
+  }
+
+  private void processIdWithoutTags(String id, String name) {
+    if (!id.endsWith(")")) {
+      this.name = name;
+    } else {
+      throw new ServiceCombException("bad format id " + id);
     }
+  }
+
+  private void processIdHadTags(String id, String[] nameAndTag) {
     this.name = nameAndTag[0];
-    this.value = value;
+    String[] tagAnValues = nameAndTag[1].split(",");
+    for (String tagAnValue : tagAnValues) {
+      String[] kv = tagAnValue.split("=");
+      if (kv.length == 2) {
+        this.tags.put(kv[0], kv[1]);
+      } else {
+        throw new ServiceCombException("bad format tag " + id);
+      }
+    }
   }
 
   public double getValue() {
@@ -58,6 +86,10 @@ public double getValue(TimeUnit unit) {
     return value;
   }
 
+  public int getTagsCount() {
+    return tags.size();
+  }
+
   public boolean containsTagKey(String tagKey) {
     return tags.containsKey(tagKey);
   }
@@ -71,11 +103,29 @@ public boolean containsTag(String tagKey, String tagValue) {
   }
 
   public boolean containsTag(String... tags) {
-    for (int i = 0; i < tags.length; i += 2) {
-      if (!containsTag(tags[i], tags[i + 1])) {
-        return false;
+    if (tags.length >= 2 && tags.length % 2 == 0) {
+      for (int i = 0; i < tags.length; i += 2) {
+        if (!containsTag(tags[i], tags[i + 1])) {
+          return false;
+        }
+      }
+      return true;
+    }
+    throw new ServiceCombException("bad tags count : " + String.join(",", tags));
+  }
+
+  private int getCharCount(String id, char c) {
+    int count = 0;
+    for (char cr : id.toCharArray()) {
+      if (cr == c) {
+        count++;
       }
     }
-    return true;
+    return count;
+  }
+
+  private boolean validateMetricId(String id) {
+    return id != null && !"".equals(id) && !id.endsWith("(") &&
+        getCharCount(id, '(') <= 1 && getCharCount(id, ')') <= 1;
   }
 }
\ No newline at end of file
diff --git a/foundations/foundation-metrics/src/test/java/org/apache/servicecomb/foundation/metrics/health/TestHealthCheckerManager.java
b/foundations/foundation-metrics/src/test/java/org/apache/servicecomb/foundation/metrics/health/TestHealthCheckerManager.java
index c8474eecf..d353660bb 100644
--- a/foundations/foundation-metrics/src/test/java/org/apache/servicecomb/foundation/metrics/health/TestHealthCheckerManager.java
+++ b/foundations/foundation-metrics/src/test/java/org/apache/servicecomb/foundation/metrics/health/TestHealthCheckerManager.java
@@ -20,6 +20,7 @@
 import java.util.Map;
 
 import org.junit.Assert;
+import org.junit.Before;
 import org.junit.Test;
 
 public class TestHealthCheckerManager {
@@ -48,32 +49,35 @@ public HealthCheckResult check() {
     }
   };
 
-  private void reset() {
+  @Before
+  public void reset() {
     HealthCheckerManager.getInstance().unregister(good.getName());
     HealthCheckerManager.getInstance().unregister(bad.getName());
   }
 
   @Test
-  public void checkResultCount() {
-    reset();
+  public void checkResultCount_None() {
     Map<String, HealthCheckResult> results = HealthCheckerManager.getInstance().check();
     Assert.assertEquals(0, results.size());
+  }
 
-    reset();
+  @Test
+  public void checkResultCount_One() {
     HealthCheckerManager.getInstance().register(good);
-    results = HealthCheckerManager.getInstance().check();
+    Map<String, HealthCheckResult> results = HealthCheckerManager.getInstance().check();
     Assert.assertEquals(1, results.size());
+  }
 
-    reset();
+  @Test
+  public void checkResultCount_Both() {
     HealthCheckerManager.getInstance().register(good);
     HealthCheckerManager.getInstance().register(bad);
-    results = HealthCheckerManager.getInstance().check();
+    Map<String, HealthCheckResult> results = HealthCheckerManager.getInstance().check();
     Assert.assertEquals(2, results.size());
   }
 
   @Test
   public void checkGoodResult() {
-    reset();
     HealthCheckerManager.getInstance().register(good);
     HealthCheckerManager.getInstance().register(bad);
     HealthCheckResult result = HealthCheckerManager.getInstance().check().get("testGood");
@@ -84,7 +88,6 @@ public void checkGoodResult() {
 
   @Test
   public void checkBadResult() {
-    reset();
     HealthCheckerManager.getInstance().register(good);
     HealthCheckerManager.getInstance().register(bad);
     HealthCheckResult result = HealthCheckerManager.getInstance().check().get("testBad");
diff --git a/foundations/foundation-metrics/src/test/java/org/apache/servicecomb/foundation/metrics/publish/TestMetric.java
b/foundations/foundation-metrics/src/test/java/org/apache/servicecomb/foundation/metrics/publish/TestMetric.java
new file mode 100644
index 000000000..eb47dd78f
--- /dev/null
+++ b/foundations/foundation-metrics/src/test/java/org/apache/servicecomb/foundation/metrics/publish/TestMetric.java
@@ -0,0 +1,118 @@
+/*
+ * 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.servicecomb.foundation.metrics.publish;
+
+import org.apache.servicecomb.foundation.common.exceptions.ServiceCombException;
+import org.junit.Assert;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+
+public class TestMetric {
+  @Test
+  public void testNewMetric() {
+
+    Metric metric = new Metric("Key", 100);
+    Assert.assertEquals(0, metric.getTagsCount());
+
+    metric = new Metric("Key(A=1)", 100);
+    Assert.assertEquals(1, metric.getTagsCount());
+    Assert.assertEquals(true, metric.containsTagKey("A"));
+
+    metric = new Metric("Key(A=1,B=X)", 100);
+    Assert.assertEquals(2, metric.getTagsCount());
+    Assert.assertEquals(true, metric.containsTagKey("A"));
+    Assert.assertEquals(true, metric.containsTagKey("B"));
+    Assert.assertEquals("1", metric.getTagValue("A"));
+    Assert.assertEquals("X", metric.getTagValue("B"));
+
+    checkBadIdFormat(null);
+    checkBadIdFormat("");
+    checkBadIdFormat("(");
+    checkBadIdFormat(")");
+    checkBadIdFormat("()");
+
+    checkBadIdFormat("Key(");
+    checkBadIdFormat("Key)");
+    checkBadIdFormat("Key()");
+
+    checkBadIdFormat("Key(X)");
+    checkBadIdFormat("Key(X");
+    checkBadIdFormat("Key(X))");
+    checkBadIdFormat("Key((X)");
+
+    checkBadIdFormat("Key(X=)");
+    checkBadIdFormat("Key(X=");
+    checkBadIdFormat("Key(X=))");
+    checkBadIdFormat("Key((X=)");
+
+    checkBadIdFormat("Key(X=,)");
+    checkBadIdFormat("Key(X=,");
+    checkBadIdFormat("Key(X=,))");
+    checkBadIdFormat("Key((X=,)");
+
+    checkBadIdFormat("Key(X=,Y)");
+    checkBadIdFormat("Key(X=,Y");
+    checkBadIdFormat("Key(X=,Y))");
+    checkBadIdFormat("Key((X=,Y)");
+
+    checkBadIdFormat("Key(X=1,Y)");
+    checkBadIdFormat("Key(X=1,Y");
+    checkBadIdFormat("Key(X=1,Y))");
+    checkBadIdFormat("Key((X=1,Y)");
+
+    checkBadIdFormat("Key(X=1))");
+    checkBadIdFormat("Key((X=1)");
+
+    checkBadIdFormat("Key(X=1) ");
+    checkBadIdFormat("Key(X=1,Y=2)Z");
+
+    checkBadIdFormat("Key(X=1)()");
+    checkBadIdFormat("Key(X=1)(Y=1)");
+  }
+
+  @Test
+  public void checkMetricContainsTag() {
+    Metric metric = new Metric("Key(A=1,B=X)", 100);
+    Assert.assertEquals(true, metric.containsTag("A", "1"));
+  }
+
+  @Test
+  public void checkMetricContainsTagWithWrongTagsCount() {
+    Metric metric = new Metric("Key(A=1,B=X)", 100);
+    checkMetricContainsTagWithWrongTagsCount(metric, "A");
+    checkMetricContainsTagWithWrongTagsCount(metric, "A", "1", "B");
+    checkMetricContainsTagWithWrongTagsCount(metric, "A", "1", "B", "X", "C");
+  }
+
+
+  @Rule
+  public ExpectedException thrown = ExpectedException.none();
+
+  private void checkMetricContainsTagWithWrongTagsCount(Metric metric, String... tags) {
+    thrown.expect(ServiceCombException.class);
+    metric.containsTag(tags);
+    Assert.fail("checkMetricContainsTagWithWrongTagsCount failed : " + String.join(",", tags));
+  }
+
+  private void checkBadIdFormat(String id) {
+    thrown.expect(ServiceCombException.class);
+    new Metric(id, 100);
+    Assert.fail("checkBadIdFormat failed : " + id);
+  }
+}
diff --git a/foundations/foundation-metrics/src/test/java/org/apache/servicecomb/foundation/metrics/publish/TestMetricNode.java
b/foundations/foundation-metrics/src/test/java/org/apache/servicecomb/foundation/metrics/publish/TestMetricNode.java
index 860113704..53427b5e1 100644
--- a/foundations/foundation-metrics/src/test/java/org/apache/servicecomb/foundation/metrics/publish/TestMetricNode.java
+++ b/foundations/foundation-metrics/src/test/java/org/apache/servicecomb/foundation/metrics/publish/TestMetricNode.java
@@ -17,7 +17,9 @@
 
 package org.apache.servicecomb.foundation.metrics.publish;
 
+import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
 
@@ -87,4 +89,21 @@ public void checkGenerateMetricNodeFromExistedNode() {
     MetricNode newNode = new MetricNode(node_k1.getMetrics(), "K2", "K3");
     Assert.assertEquals(1, newNode.getChildrenNode("2").getChildrenNode("3").getMetricCount(),
0);
   }
-}
\ No newline at end of file
+
+  @Test
+  public void testNewMetricNode() {
+    List<Metric> metrics = new ArrayList<>();
+    metrics.add(new Metric("Y(K1=1,K2=2,K3=3)", 1));
+    metrics.add(new Metric("Y(K1=1,K2=20,K3=30)", 10));
+    metrics.add(new Metric("Y(K1=10,K2=20,K3=300)", 100));
+    metrics.add(new Metric("Y(K1=10,K2=20,K3=3000)", 1000));
+
+    MetricNode node = new MetricNode(metrics);
+    Assert.assertEquals(4, node.getMetricCount());
+    Assert.assertEquals(1.0, node.getFirstMatchMetricValue("K3", "3"), 0);
+
+    node = new MetricNode(metrics, "K1");
+    Assert.assertEquals(2, node.getChildrenCount());
+    Assert.assertEquals(1.0, node.getChildren("1").getFirstMatchMetricValue("K3", "3"), 0);
+  }
+}
diff --git a/foundations/foundation-metrics/src/test/java/org/apache/servicecomb/foundation/metrics/publish/TestMetricsLoader.java
b/foundations/foundation-metrics/src/test/java/org/apache/servicecomb/foundation/metrics/publish/TestMetricsLoader.java
index 917d9e561..c3eed5ebe 100644
--- a/foundations/foundation-metrics/src/test/java/org/apache/servicecomb/foundation/metrics/publish/TestMetricsLoader.java
+++ b/foundations/foundation-metrics/src/test/java/org/apache/servicecomb/foundation/metrics/publish/TestMetricsLoader.java
@@ -20,9 +20,12 @@
 import java.util.HashMap;
 import java.util.Map;
 
+import org.apache.servicecomb.foundation.common.exceptions.ServiceCombException;
 import org.junit.Assert;
 import org.junit.BeforeClass;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 
 public class TestMetricsLoader {
   private static MetricsLoader loader;
@@ -53,4 +56,14 @@ public void checkGetChildrenCount() {
     MetricNode node = loader.getMetricTree("X", "K1");
     Assert.assertEquals(2, node.getChildrenCount());
   }
+
+  @Rule
+  public ExpectedException thrown = ExpectedException.none();
+
+  @Test
+  public void checkNoSuchId() {
+    thrown.expect(ServiceCombException.class);
+    loader.getMetricTree("Z");
+    Assert.fail("checkNoSuchId failed");
+  }
 }
diff --git a/metrics/metrics-core/src/main/java/org/apache/servicecomb/metrics/core/MonitorManager.java
b/metrics/metrics-core/src/main/java/org/apache/servicecomb/metrics/core/MonitorManager.java
index d9832160f..110555fd9 100644
--- a/metrics/metrics-core/src/main/java/org/apache/servicecomb/metrics/core/MonitorManager.java
+++ b/metrics/metrics-core/src/main/java/org/apache/servicecomb/metrics/core/MonitorManager.java
@@ -17,7 +17,9 @@
 
 package org.apache.servicecomb.metrics.core;
 
+import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.SortedMap;
@@ -25,7 +27,9 @@
 import java.util.concurrent.Callable;
 import java.util.function.Function;
 
+import org.apache.commons.lang3.StringUtils;
 import org.apache.servicecomb.foundation.common.concurrent.ConcurrentHashMapEx;
+import org.apache.servicecomb.foundation.common.exceptions.ServiceCombException;
 import org.apache.servicecomb.foundation.metrics.MetricsConst;
 
 import com.netflix.config.DynamicPropertyFactory;
@@ -56,6 +60,8 @@
 
   private final MonitorRegistry basicMonitorRegistry;
 
+  private static final char[] forbiddenCharacter = new char[] {'(', ')', '=', ','};
+
   private static final MonitorManager INSTANCE = new MonitorManager();
 
   public static MonitorManager getInstance() {
@@ -78,6 +84,7 @@ private void setupWindowTime() {
   }
 
   public Counter getCounter(String name, String... tags) {
+    validateMonitorNameAndTags(name, tags);
     return counters.computeIfAbsent(getMonitorKey(name, tags), f -> {
       Counter counter = new BasicCounter(getConfig(name, tags));
       basicMonitorRegistry.register(counter);
@@ -86,6 +93,7 @@ public Counter getCounter(String name, String... tags) {
   }
 
   public Counter getCounter(Function<MonitorConfig, Counter> function, String name,
String... tags) {
+    validateMonitorNameAndTags(name, tags);
     return counters.computeIfAbsent(getMonitorKey(name, tags), f -> {
       Counter counter = function.apply(getConfig(name, tags));
       basicMonitorRegistry.register(counter);
@@ -94,6 +102,7 @@ public Counter getCounter(Function<MonitorConfig, Counter> function,
String name
   }
 
   public MaxGauge getMaxGauge(String name, String... tags) {
+    validateMonitorNameAndTags(name, tags);
     return maxGauges.computeIfAbsent(getMonitorKey(name, tags), f -> {
       MaxGauge maxGauge = new MaxGauge(getConfig(name, tags));
       basicMonitorRegistry.register(maxGauge);
@@ -102,6 +111,7 @@ public MaxGauge getMaxGauge(String name, String... tags) {
   }
 
   public <V extends Number> Gauge getGauge(Callable<V> callable, String name,
String... tags) {
+    validateMonitorNameAndTags(name, tags);
     return gauges.computeIfAbsent(getMonitorKey(name, tags), f -> {
       Gauge gauge = new BasicGauge<>(getConfig(name, tags), callable);
       basicMonitorRegistry.register(gauge);
@@ -110,6 +120,7 @@ public MaxGauge getMaxGauge(String name, String... tags) {
   }
 
   public Timer getTimer(String name, String... tags) {
+    validateMonitorNameAndTags(name, tags);
     return timers.computeIfAbsent(getMonitorKey(name, tags), f -> {
       Timer timer = new BasicTimer(getConfig(name, tags));
       basicMonitorRegistry.register(timer);
@@ -127,6 +138,7 @@ public Timer getTimer(String name, String... tags) {
   }
 
   private MonitorConfig getConfig(String name, String... tags) {
+    validateMonitorNameAndTags(name, tags);
     Builder builder = MonitorConfig.builder(name);
     for (int i = 0; i < tags.length; i += 2) {
       builder.withTag(tags[i], tags[i + 1]);
@@ -135,6 +147,7 @@ private MonitorConfig getConfig(String name, String... tags) {
   }
 
   private String getMonitorKey(String name, String... tags) {
+    validateMonitorNameAndTags(name, tags);
     if (tags.length != 0) {
       SortedMap<String, String> tagMap = new TreeMap<>();
       for (int i = 0; i < tags.length; i += 2) {
@@ -152,16 +165,15 @@ private String getMonitorKey(String name, String... tags) {
   }
 
   private String getMonitorKey(MonitorConfig config) {
-    TagList tags = config.getTags();
-    StringBuilder tagPart = new StringBuilder("(");
-    for (Tag tag : tags) {
+    TagList tagList = config.getTags();
+    List<String> tags = new ArrayList<>();
+    for (Tag tag : tagList) {
       if (!"type".equals(tag.getKey())) {
-        tagPart.append(String.format("%s=%s,", tag.getKey(), tag.getValue()));
+        tags.add(tag.getKey());
+        tags.add(tag.getValue());
       }
     }
-    tagPart.deleteCharAt(tagPart.length() - 1);
-    tagPart.append(")");
-    return config.getName() + tagPart.toString();
+    return getMonitorKey(config.getName(), tags.toArray(new String[0]));
   }
 
 
@@ -182,4 +194,24 @@ private void registerSystemMetrics() {
   private <V extends Number> void registerSystemMetricItem(Callable<V> callable,
String name) {
     this.getGauge(callable, MetricsConst.JVM, MetricsConst.TAG_STATISTIC, "gauge", MetricsConst.TAG_NAME,
name);
   }
+
+  private void validateMonitorNameAndTags(String name, String... tags) {
+    boolean passed = StringUtils.isNotEmpty(name) && tags.length % 2 == 0;
+    if (passed) {
+      if (StringUtils.containsNone(name, forbiddenCharacter)) {
+        for (String tag : tags) {
+          if (StringUtils.containsAny(tag, forbiddenCharacter)) {
+            throw new ServiceCombException(
+                "validate name and tags failed name = " + name + " tags = " + String.join(",",
tags));
+          }
+        }
+      } else {
+        throw new ServiceCombException(
+            "validate name and tags failed name = " + name + " tags = " + String.join(",",
tags));
+      }
+    } else {
+      throw new ServiceCombException(
+          "validate name and tags failed name = " + name + " tags = " + String.join(",",
tags));
+    }
+  }
 }
\ No newline at end of file
diff --git a/metrics/metrics-core/src/test/java/org/apache/servicecomb/metrics/core/TestHealthCheckerPublisher.java
b/metrics/metrics-core/src/test/java/org/apache/servicecomb/metrics/core/TestHealthCheckerPublisher.java
index 613b252ef..d0c182e2d 100644
--- a/metrics/metrics-core/src/test/java/org/apache/servicecomb/metrics/core/TestHealthCheckerPublisher.java
+++ b/metrics/metrics-core/src/test/java/org/apache/servicecomb/metrics/core/TestHealthCheckerPublisher.java
@@ -24,46 +24,60 @@
 import org.apache.servicecomb.foundation.metrics.health.HealthCheckerManager;
 import org.apache.servicecomb.metrics.core.publish.HealthCheckerPublisher;
 import org.junit.Assert;
-import org.junit.BeforeClass;
+import org.junit.Before;
 import org.junit.Test;
 
 public class TestHealthCheckerPublisher {
+  private HealthChecker good = new HealthChecker() {
+    @Override
+    public String getName() {
+      return "test";
+    }
 
-  @BeforeClass
-  public static void setup() {
-    HealthCheckerManager.getInstance().register(new HealthChecker() {
-      @Override
-      public String getName() {
-        return "test";
-      }
+    @Override
+    public HealthCheckResult check() {
+      return new HealthCheckResult(true, "info", "extra data");
+    }
+  };
 
-      @Override
-      public HealthCheckResult check() {
-        return new HealthCheckResult(true, "info", "extra data");
-      }
-    });
+  private HealthChecker bad = new HealthChecker() {
+    @Override
+    public String getName() {
+      return "test2";
+    }
 
-    HealthCheckerManager.getInstance().register(new HealthChecker() {
-      @Override
-      public String getName() {
-        return "test2";
-      }
+    @Override
+    public HealthCheckResult check() {
+      return new HealthCheckResult(false, "info2", "extra data 2");
+    }
+  };
 
-      @Override
-      public HealthCheckResult check() {
-        return new HealthCheckResult(false, "info2", "extra data 2");
-      }
-    });
+
+  @Before
+  public void reset() {
+    HealthCheckerManager.getInstance().unregister(good.getName());
+    HealthCheckerManager.getInstance().unregister(bad.getName());
+  }
+
+  @Test
+  public void checkHealthGood() {
+    HealthCheckerManager.getInstance().register(good);
+    HealthCheckerPublisher publisher = new HealthCheckerPublisher();
+    Assert.assertEquals(true, publisher.checkHealth());
   }
 
   @Test
-  public void checkHealth() {
+  public void checkHealthBad() {
+    HealthCheckerManager.getInstance().register(good);
+    HealthCheckerManager.getInstance().register(bad);
     HealthCheckerPublisher publisher = new HealthCheckerPublisher();
     Assert.assertEquals(false, publisher.checkHealth());
   }
 
   @Test
   public void checkHealthDetails() {
+    HealthCheckerManager.getInstance().register(good);
+    HealthCheckerManager.getInstance().register(bad);
     HealthCheckerPublisher publisher = new HealthCheckerPublisher();
     Map<String, HealthCheckResult> content = publisher.checkHealthDetails();
     Assert.assertEquals(true, content.get("test").isHealthy());
diff --git a/metrics/metrics-core/src/test/java/org/apache/servicecomb/metrics/core/TestMonitorManager.java
b/metrics/metrics-core/src/test/java/org/apache/servicecomb/metrics/core/TestMonitorManager.java
index 3f9e71463..eedcf1bc1 100644
--- a/metrics/metrics-core/src/test/java/org/apache/servicecomb/metrics/core/TestMonitorManager.java
+++ b/metrics/metrics-core/src/test/java/org/apache/servicecomb/metrics/core/TestMonitorManager.java
@@ -24,13 +24,18 @@
 import org.apache.servicecomb.core.metrics.InvocationStartExecutionEvent;
 import org.apache.servicecomb.core.metrics.InvocationStartedEvent;
 import org.apache.servicecomb.foundation.common.event.EventBus;
+import org.apache.servicecomb.foundation.common.exceptions.ServiceCombException;
 import org.apache.servicecomb.foundation.metrics.MetricsConst;
 import org.apache.servicecomb.foundation.metrics.publish.MetricNode;
 import org.apache.servicecomb.foundation.metrics.publish.MetricsLoader;
 import org.apache.servicecomb.swagger.invocation.InvocationType;
 import org.junit.Assert;
 import org.junit.BeforeClass;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
+
+import com.netflix.servo.monitor.Counter;
 
 public class TestMonitorManager {
 
@@ -262,4 +267,55 @@ public void checkFun4WaitInQueue() {
         .getChildrenNode(MetricsConst.STAGE_QUEUE);
     Assert.assertEquals(1, node4_queue.getMatchStatisticMetricValue("waitInQueue"), 0);
   }
+
+  @Test
+  public void checkRegisterMonitorWithoutAnyTags() {
+    Counter counter = MonitorManager.getInstance().getCounter("MonitorWithoutAnyTag");
+    counter.increment(999);
+    Assert.assertTrue(MonitorManager.getInstance().measure().containsKey("MonitorWithoutAnyTag"));
+    Assert.assertEquals(999, MonitorManager.getInstance().measure().get("MonitorWithoutAnyTag"),
0);
+  }
+
+  @Rule
+  public ExpectedException thrown = ExpectedException.none();
+
+  @Test
+  public void checkRegisterMonitor() {
+    MonitorManager.getInstance().getCounter("Monitor", "X", "Y");
+  }
+
+  @Test
+  public void checkRegisterMonitorWithBadTags() {
+    thrown.expect(ServiceCombException.class);
+    MonitorManager.getInstance().getCounter("MonitorWithBadCountTag", "X");
+    Assert.fail("checkRegisterMonitorWithBadTags failed");
+  }
+
+  @Test
+  public void checkRegisterMonitorWithBadNameAndTags_ContainLeftParenthesisChar() {
+    thrown.expect(ServiceCombException.class);
+    MonitorManager.getInstance().getCounter("MonitorWithBad(");
+    Assert.fail("checkRegisterMonitorWithBadTags failed : MonitorWithBad(");
+  }
+
+  @Test
+  public void checkRegisterMonitorWithBadNameAndTags_ContainCommaChar() {
+    thrown.expect(ServiceCombException.class);
+    MonitorManager.getInstance().getCounter("MonitorWithBad,");
+    Assert.fail("checkRegisterMonitorWithBadTags failed : MonitorWithBad,");
+  }
+
+  @Test
+  public void checkRegisterMonitorWithBadNameAndTags_ContainEqualChar() {
+    thrown.expect(ServiceCombException.class);
+    MonitorManager.getInstance().getCounter("MonitorWithBad", "TagX=", "Y");
+    Assert.fail("checkRegisterMonitorWithBadTags failed : name = MonitorWithBad, tags = TagX=,Y");
+  }
+
+  @Test
+  public void checkRegisterMonitorWithBadNameAndTags_ContainRightParenthesisChar() {
+    thrown.expect(ServiceCombException.class);
+    MonitorManager.getInstance().getCounter("MonitorWithBad", "TagX", "Y)");
+    Assert.fail("checkRegisterMonitorWithBadTags failed : name = MonitorWithBad, tags = TagX,Y)");
+  }
 }
\ No newline at end of file


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services

Mime
View raw message