[SCB-150] Add Status(success/failed) dimension to operation call count and tps by zhengyangyong · Pull Request #508 · apache/servicecomb-java-chassis

zhengyangyong · 2018-01-12T08:19:45Z

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.

Add Status(success/failed) dimension to operation call count and tps,now they will split into three metrics like:
servicecomb.instance.producer.producerCall.tps.{Status=all}
servicecomb.instance.producer.producerCall.tps.{Status=success}
servicecomb.instance.producer.producerCall.tps.{Status=failed}
servicecomb.instance.producer.producerCall.total.{Status=all}
servicecomb.instance.producer.producerCall.total.{Status=success}
servicecomb.instance.producer.producerCall.total.{Status=failed}
consumerCall also applied

In prometheus will use Label show this dimension:
servicecomb_calculator_calculatorRestEndpoint_calculate_producer_producerCall_tps{Status="success",instance="localhost:9696",job="servicecomb"}
servicecomb_calculator_calculatorRestEndpoint_calculate_producer_producerCall_tps{Status="failed",instance="localhost:9696",job="servicecomb"}
servicecomb_calculator_calculatorRestEndpoint_calculate_producer_producerCall_tps{Status="all",instance="localhost:9696",job="servicecomb"}

Update
now support setting output level of status dimension :

servicecomb:
  metrics:
    dimension:
      #status dimension
      status:
        # success_failed  : only output success & failed (default)
        # code_group : output 1xx,2xx,3xx,4xx,5xx and xxx
        # code       : output detail code like 200,404,500... etc
        output_level: success_failed

coveralls · 2018-01-12T08:42:43Z

Coverage increased (+0.0004%) to 87.34% when pulling 62f9984 on zhengyangyong:SCB-150 into 361c73c on apache:master.

wujimin · 2018-01-12T13:30:14Z

maybe it's better add statusCode dimension, not succ/fail

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

zhengyangyong · 2018-01-15T01:23:53Z

Package Name Updated

coveralls · 2018-01-15T01:47:43Z

Coverage increased (+0.006%) to 87.362% when pulling 3c612f4 on zhengyangyong:SCB-150 into 18847ab on apache:master.

zhengyangyong · 2018-01-15T09:41:22Z

maybe it's better add statusCode dimension, not succ/fail
@wujimin I just think it's necessary split them so detail ? @WillemJiang @seanyinx what's your suggestion thanks

WillemJiang · 2018-01-16T00:53:22Z

There could be lots's of dimensions if we create the counter per status code. My suggestion is we only provide 2xx, 3xx, 4xx, 5xx status code for it. BTW, we can take the Spring Boot actuator as an example, we don't need lots detail metrics, we just need to tell the administrator something is wrong, and you can find more information by looking up the access log.

wujimin · 2018-01-16T01:18:52Z

2xx/3xx/4xx/5xx almost equals statusCode
and 3xx need to support special logic: 304 belongs success

i thinks, for metrics kernel data, we care for statusCode
but for data published interface, we can provide different model for different scenes

zhengyangyong · 2018-01-16T12:01:22Z

I think I can add a switch named servicecomb.metrics.dimension.http_code,default value is false,if user enable it,detail metrics will be record like :
servicecomb.instance.producer.producerCall.tps.{Status=all}
servicecomb.instance.producer.producerCall.tps.{Status=success}
servicecomb.instance.producer.producerCall.tps.{Status=failed}
servicecomb.instance.producer.producerCall.tps.{Status=200}
servicecomb.instance.producer.producerCall.tps.{Status=404}
...

zhengyangyong · 2018-01-16T12:09:59Z

I also think most of the time user would like use metrics outputs draw curves directly and don't want do any more aggregation by themself ,only detail items provided may not good idea.

wujimin · 2018-01-17T00:40:20Z

not "only detail items "
detail is our kernel data, by this data, we can summarize everything we want.

seanyinx · 2018-01-17T01:31:45Z

...ics/metrics-common/src/main/java/org/apache/servicecomb/metrics/common/MetricsDimension.java

+          MetricsDimension.DIMENSION_STATUS_SUCCESS,
+          MetricsDimension.DIMENSION_STATUS_FAILED};
+    }
+    throw new ServiceCombException("illegal dimension key : " + dimension);


if there's only one dimension, there's no need to pass in the parameter. if there are multiple, returning an empty array is preferable

We will support more dimensions later, and dimension key must correct

seanyinx · 2018-01-17T01:33:34Z

metrics/metrics-core/src/main/java/org/apache/servicecomb/metrics/core/utils/MonitorUtils.java

+      }
+      return tagMap;
+    }
+    return null;


returning Collections.emptyMap() is preferable

seanyinx · 2018-01-17T01:35:46Z

...ics-prometheus/src/main/java/org/apache/servicecomb/metrics/prometheus/MetricsCollector.java

+    familySamples.add(new MetricFamilySamples("Instance Level", Type.UNTYPED, "Instance Level Metrics", samples));
+
+    if (registryMetric.getConsumerMetrics().size() != 0) {
+      samples = new ArrayList<>();


use a local list in this scope is better

seanyinx · 2018-01-17T01:36:14Z

...ics-prometheus/src/main/java/org/apache/servicecomb/metrics/prometheus/MetricsCollector.java

-          .add(getFamilySamples(producerMetric.getKey() + " Producer Side", producerMetric.getValue().toMap()));
+
+    if (registryMetric.getProducerMetrics().size() != 0) {
+      samples = new ArrayList<>();


same as above

seanyinx · 2018-01-17T01:39:12Z

...cs/metrics-common/src/main/java/org/apache/servicecomb/metrics/common/DoubleMetricValue.java

+    if (this.getKey().equals(value.getKey())) {
+      return new DoubleMetricValue(this.getKey(), this.getValue() + value.getValue(), this.getDimensions());
+    }
+    throw new ServiceCombException("unable merge different key values,source key :" + value.getKey() +


any place to catch this exception? we don't want to interrupt user business logic

seanyinx · 2018-01-17T01:45:53Z

agreed with @zhengyangyong on option to enable/disable metrics collection based on status code and @WillemJiang on metrics on status range instead of exact status code. we have to be aware of framework memory footprint.

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

zhengyangyong · 2018-01-17T07:40:52Z

now support setting output level of status dimension :

servicecomb:
  metrics:
    dimension:
      #status dimension
      status:
        # success_failed  : only output success & failed (default)
        # code_group : output 1xx,2xx,3xx,4xx,5xx and xxx
        # code       : output detail code like 200,404,500... etc
        output_level: success_failed

coveralls · 2018-01-17T12:31:16Z

Coverage decreased (-0.02%) to 87.337% when pulling 63e9371 on zhengyangyong:SCB-150 into 18847ab on apache:master.

coveralls · 2018-01-17T14:12:10Z

Coverage decreased (-0.05%) to 87.308% when pulling 63e9371 on zhengyangyong:SCB-150 into 18847ab on apache:master.

wujimin · 2018-01-18T00:59:48Z

will this make our logic too complex?
our metrics will not record history data, only some summaries, there is no memory problem?
if kernel data is about statusCode, then metrics is very simple
just when publish data, need to publish different model like: succ/fail, 2xx/3xx/4xx......

zhengyangyong added 8 commits January 15, 2018 08:46

SCB-150 add success/failed dimension for call count and tps

1a151ac

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

SCB-150 impl success /failed in invocation and event

5c74581

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

SCB-150 minor refactor and fix test case

8a78cb8

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

SCB-150 minor refactor and fix test case

437f803

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

SCB-150 minor refactor for prometheus

5659a4d

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

SCB-150 add module name

bf0e84d

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

SCB-150 fix RegistryMetric serialization

17f62f3

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

SCB-150 update package name and resolve conflicts

3c612f4

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

zhengyangyong force-pushed the SCB-150 branch from 62f9984 to 3c612f4 Compare January 15, 2018 01:23

seanyinx reviewed Jan 17, 2018

View reviewed changes

zhengyangyong added 2 commits January 17, 2018 10:19

SCB-150 fix pr comment

1e0da7a

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

SCB-150 add Status Dimension output level support

63e9371

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

seanyinx approved these changes Jan 23, 2018

View reviewed changes

liubao68 approved these changes Jan 23, 2018

View reviewed changes

WillemJiang merged commit bd87a72 into apache:master Jan 24, 2018

zhengyangyong deleted the SCB-150 branch March 27, 2018 06:30

Conversation

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

Uh oh!

Uh oh!

coveralls commented Jan 12, 2018

Uh oh!

wujimin commented Jan 12, 2018

Uh oh!

zhengyangyong commented Jan 15, 2018

Uh oh!

coveralls commented Jan 15, 2018

Uh oh!

zhengyangyong commented Jan 15, 2018

Uh oh!

WillemJiang commented Jan 16, 2018

Uh oh!

wujimin commented Jan 16, 2018

Uh oh!

zhengyangyong commented Jan 16, 2018

Uh oh!

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

Uh oh!

Uh oh!

wujimin commented Jan 17, 2018

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!

seanyinx commented Jan 17, 2018

Uh oh!

zhengyangyong commented Jan 17, 2018

Uh oh!

coveralls commented Jan 17, 2018

Uh oh!

coveralls commented Jan 17, 2018

Uh oh!

wujimin commented Jan 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zhengyangyong commented Jan 12, 2018 •

edited

Loading

zhengyangyong commented Jan 16, 2018 •

edited

Loading