[SCB-358] fix bug for monitor output id that register only name without any tags by zhengyangyong · Pull Request #562 · apache/servicecomb-java-chassis

zhengyangyong · 2018-02-28T08:07:02Z

Signed-off-by: zhengyangyong yangyong.zheng@huawei.com

Follow this checklist to help us incorporate your contribution quickly and easily:

Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [SCB-XXX] Fixes bug in ApproximateQuantiles, where you replace SCB-XXX with the appropriate JIRA issue.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean install to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

User can register monitor only name without any tags like :
MonitorManager.getInstance().getCounter("MonitorWithoutAnyTag");
In this case output metric id will be MonitorWithoutAnyTag) , correct output metric id is MonitorWithoutAnyTag ,not include a ')'

Also fix MetricsLoader load this style of metric id.

Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>

coveralls · 2018-02-28T08:38:45Z

Coverage increased (+0.04%) to 87.247% when pulling ea0e61c on zhengyangyong:SCB-358 into b09a6c2 on apache:master.

Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>

…gs() Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>

Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>

WillemJiang

Please fix the issues of the comments.

WillemJiang · 2018-02-28T15:42:46Z

  @Test
-  public void checkHealth() {
+  public void checkHealthBad() {
+    reset();


Using the method with Setup to do work （annotation Before）

WillemJiang · 2018-02-28T15:44:01Z

    this.getGauge(callable, MetricsConst.JVM, MetricsConst.TAG_STATISTIC, "gauge", MetricsConst.TAG_NAME, name);
  }
+
+  private boolean isCorrectMonitorNameAndTags(String name, String... tags) {


isCorrect -> validate

WillemJiang · 2018-02-28T15:47:57Z

+  public void checkUnmatchTypeWillNotReceived() throws InterruptedException {
+    AtomicBoolean eventReceived = new AtomicBoolean(false);
+
+    EventListener<String> listener = new EventListener<String>() {


The EventListener can be reused out of the checkUmatchType method.

WillemJiang · 2018-02-28T15:48:48Z

-    String[] tagAnValues = nameAndTag[1].split("[=,)]");
-    for (int i = 0; i < tagAnValues.length; i += 2) {
-      this.tags.put(tagAnValues[i], tagAnValues[i + 1]);
+    if (isCorrectId(id)) {


isCorrectId -> validateMetricId.

WillemJiang · 2018-02-28T15:49:45Z

+        if (!id.endsWith(")")) {
+          this.name = nameAndTag[0];
+        } else {
+          throw new ServiceCombException("bad format id");


Please add the id the message, otherwise the user cannot know which id is wrong.

WillemJiang · 2018-02-28T16:02:50Z

+    return count;
+  }
+
+  private boolean isCorrectId(String id) {


what if the id is empty string such as "".

WillemJiang · 2018-02-28T16:03:49Z

+  private void checkBadIdFormat(String id) throws Exception {
+    try {
+      new Metric(id, 100);
+      throw new Exception("CheckFailed");


You can use fail("xxx") here.

WillemJiang · 2018-02-28T16:05:28Z

+    }
+  }
+
+  private void checkBadIdFormat(String id) throws Exception {


Using the Expected Exception can do the work.
https://github.com/junit-team/junit4/wiki/exception-testing

WillemJiang · 2018-02-28T16:06:32Z

+    }
+    //ignore because throw exception is correct
+    catch (ServiceCombException ignore) {
+    }


We need to verify the ServiceCombException message here.

WillemJiang · 2018-02-28T16:07:12Z

-      basicMonitorRegistry.register(counter);
-      return counter;
-    });
+    if (isCorrectMonitorNameAndTags(name, tags)) {


It's more easy to throw the exception here.

Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>

wujimin · 2018-03-01T00:37:21Z

-    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)) {


so many if/else...... flat them.

wujimin · 2018-03-01T00:39:17Z

-    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)) {


why need to validate id?
servo/spectator's id, we can not use?

Metric、MetricNode和MetricLoader是工具类用于分析从publish获取的metrics结果Map<String,Double>，如果从servo的config获取会更简单一点，现在从String id parse 比如servicecomb.invocation(operation={operationName},role={role},stage={stage},statistic={statistic},status={status},unit={unit})也是有必要的，我会在新的PR增加你说的内容

i will remove metric.java in new PR.

wujimin · 2018-03-01T00:41:18Z

+    if (validateMetricId(id)) {
+      this.tags = new HashMap<>();
+      this.value = value;
+      String[] nameAndTag = id.split("[()]");


why need to split tags?
servo/spectator's already have tags information.

if lost these informations caused by our mechanism, then we should change our mechanism.
this is why i DO NOT agree work on current mechanism.

我会在新的PR增加从MonitorConfig创建Metric，不必parse

i will remove metric.java in new PR.

wujimin · 2018-03-01T00:45:19Z

@@ -81,4 +73,14 @@ public void process(String data) {
    Thread.sleep(1000);


why sleep?
our case run too fast????????????????????????????

Fixed,removed

Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>

…e contract changes but the version number is not changed

…e contract changes but the version number is not changed fixed

SCB-358 fix bug for monitor register only name without any tags

cd7e6c2

Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>

zhengyangyong added 3 commits February 28, 2018 22:13

SCB-358 add more test case

ecd7329

Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>

SCB-358 change checkMonitorNameAndTags() to isCorrectMonitorNameAndTa…

65dba48

…gs() Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>

SCB-358 add MonitorManager get monitor name and tag args check

6f9c6ea

Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>

WillemJiang requested changes Feb 28, 2018

View reviewed changes

SCB-358 fix pr comment

1f5496b

Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>

zhengyangyong force-pushed the SCB-358 branch from 3014285 to 1f5496b Compare March 1, 2018 00:08

wujimin reviewed Mar 1, 2018

View reviewed changes

SCB-358 fix pr comment

ea0e61c

Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>

zhengyangyong force-pushed the SCB-358 branch from 9ebece7 to ea0e61c Compare March 1, 2018 03:13

WillemJiang approved these changes Mar 1, 2018

View reviewed changes

WillemJiang merged commit 255f86e into apache:master Mar 1, 2018

zhengyangyong deleted the SCB-358 branch March 27, 2018 06:29

hujinming178 added a commit to hujinming178/servicecomb-java-chassis that referenced this pull request Jul 11, 2022

[apache#562]Spring Cloud Huawei automatically updates contracts if th…

57618e4

…e contract changes but the version number is not changed

hujinming178 added a commit to hujinming178/servicecomb-java-chassis that referenced this pull request Jul 15, 2022

[apache#562]Spring Cloud Huawei automatically updates contracts if th…

8ea8d81

…e contract changes but the version number is not changed fixed

hujinming178 added a commit to hujinming178/servicecomb-java-chassis that referenced this pull request Jul 20, 2022

[apache#562]Spring Cloud Huawei automatically updates contracts if th…

5b5822b

…e contract changes but the version number is not changed fixed

		@@ -81,4 +73,14 @@ public void process(String data) {
		Thread.sleep(1000);

Conversation

zhengyangyong commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WillemJiang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zhengyangyong commented Feb 28, 2018 •

edited

Loading

coveralls commented Feb 28, 2018 •

edited

Loading