[SCB-327] Update metrics publish data module (Re-organized) #555

zhengyangyong · 2018-02-12T03:36:47Z

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.

Re-organize commits of PR : #550

Adjust metrics publish format to spring cloud netflix style
Before:
servicecomb.instance.system.cpu.load
servicecomb.instance.system.cpu.runningThreads
servicecomb.instance.system.heap.init
servicecomb.instance.system.heap.max
servicecomb.instance.system.heap.commit
servicecomb.instance.system.heap.used
servicecomb.instance.system.nonHeap.init
servicecomb.instance.system.nonHeap.max
servicecomb.instance.system.nonHeap.commit
servicecomb.instance.system.nonHeap.used
servicecomb.instance | operationName.producer.waitInQueue.count
servicecomb.instance | operationName.producer.lifeTimeInQueue.average
servicecomb.instance | operationName.producer.lifeTimeInQueue.max
servicecomb.instance | operationName.producer.lifeTimeInQueue.min
servicecomb.instance | operationName.producer.executionTime.average
servicecomb.instance | operationName.producer.executionTime.max
servicecomb.instance | operationName.producer.executionTime.min
servicecomb.instance | operationName.producer.producerLatency.average
servicecomb.instance | operationName.producer.producerLatency.max
servicecomb.instance | operationName.producer.producerLatency.min
servicecomb.instance | operationName.producer.producerCall.total
servicecomb.instance | operationName.producer.producerCall.tps
servicecomb.instance | operationName.consumer.consumerLatency.average
servicecomb.instance | operationName.consumer.consumerLatency.max
servicecomb.instance | operationName.consumer.consumerLatency.min
servicecomb.instance | operationName.consumer.consumerCall.total
servicecomb.instance | operationName.consumer.consumerCall.tps

Current:
jvm(statistic=gauge,name={name})
{name} include :
cpuLoad,cpuRunningThreads,heapInit,heapMax,heapCommit,
heapUsed,nonHeapInit,nonHeapMax,nonHeapCommit,nonHeapUsed

servicecomb.invocation(operation={operationName},role={role},stage={stage},statistic={statistic},status={status},unit={unit})

TagName	TagValue
operation	Operation Name
role	consume,producer
stage	queue,execution,total
statistic	tps,count,max,waitInQueue,latency
status	statusCode like 200, 404 ... etc
unit	time unit like MILLISECONDS etc

coveralls · 2018-02-12T04:08:38Z

Coverage increased (+0.08%) to 87.208% when pulling f7f3843 on zhengyangyong:SCB-327-2 into b0f8224 on apache:master.

WillemJiang · 2018-02-14T01:39:43Z

core/src/main/java/org/apache/servicecomb/core/Invocation.java

-            this.invocationType, finishedTime - startProcessingTime,
-            finishedTime - startTime, statusCode, success));
+        .triggerEvent(new InvocationFinishedEvent(operationMeta.getMicroserviceQualifiedName(), this.invocationType,
+            startProcessingTime - startTime,


Can you explain the meaning of startTime, startProcessingTime?

On producer side , startTime is when invocation received,startProcessingTime is when invocation fetch from queue and start execute;

WillemJiang · 2018-02-14T01:55:21Z

...main/java/org/apache/servicecomb/foundation/metrics/publish/DefaultHealthCheckExtraData.java

@@ -15,7 +15,7 @@
 * limitations under the License.
 */

-package org.apache.servicecomb.metrics.common;
+package org.apache.servicecomb.foundation.metrics.publish;

 public class DefaultHealthCheckExtraData {


What's the relationship between the DefaultHealthCheckExtraData and HealthCheckResult.extraData?
We don't need to use the String to store result, it's easy to use toString method to turn the instance of HealthCheckExtraData into a string.

We may no need provider this default health check because :
1.make some confusion
2.Service Center had same function

Had removed

WillemJiang · 2018-02-14T02:06:00Z

...tion-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/publish/MetricNode.java

+
+import org.apache.servicecomb.foundation.metrics.MetricsConst;
+
+public class MetricNode {


Can you show me the unit test of MetricNode?

WillemJiang · 2018-02-14T02:08:04Z

...n-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/publish/MetricsLoader.java

+    if (metrics.containsKey(id)) {
+      return new MetricNode(metrics.get(id), groupTagKeys);
+    }
+    return null;


I'm not a big fan of null, you can throw exception or using Option return the value.

WillemJiang · 2018-02-14T02:19:15Z

...n/java/org/apache/servicecomb/metrics/core/event/InvocationStartProcessingEventListener.java

@@ -43,7 +43,6 @@ public void process(Event data) {
    if (InvocationType.PRODUCER.equals(event.getInvocationType())) {
      ProducerInvocationMonitor monitor = registryMonitor.getProducerInvocationMonitor(event.getOperationName());
      monitor.getWaitInQueue().increment(-1);
-      monitor.getLifeTimeInQueue().update(event.getInQueueNanoTime());


Why did you remove this line?

queue stage latency had move from this event to finish event

WillemJiang · 2018-02-14T02:31:32Z

...ore/src/main/java/org/apache/servicecomb/metrics/core/monitor/ConsumerInvocationMonitor.java


-public class ConsumerInvocationMonitor extends InvocationMonitor {
+public class ConsumerInvocationMonitor {


What we should do if we want to change the ConsumerInvocationMonitor into ProducerInvocationMonitor？We don't need to write lots Classes here, please use abstraction to reduce the number of the class.

I had use DefaultMonitorRegistry as monitor container replace it;
In order to update monitor,I had add a group of basic wrapped monitor extends from AbstractMonitor;
Measure metrics will direct from DefaultMonitorRegistry.getInstance().getRegisteredMonitors() then getValue()

WillemJiang · 2018-02-14T02:34:21Z

.../metrics-core/src/main/java/org/apache/servicecomb/metrics/core/monitor/RegistryMonitor.java

-  public RegistryMetric toRegistryMetric(int windowTimeIndex) {
-    Map<String, ConsumerInvocationMetric> consumerInvocationMetrics = new HashMap<>();
+  public Map<String, Double> measure(int windowTimeIndex, boolean calculateLatency) {
+    Map<String, Double> measurements = new HashMap<>(systemMonitor.measure());


If we want measure the invocation monitor, we don't need to know if it consumerInvocationMonitor or producerInvocationMonitor.

WillemJiang · 2018-02-22T08:18:19Z

common/common-rest/src/main/java/org/apache/servicecomb/common/rest/AbstractRestInvocation.java

@@ -114,8 +113,8 @@ public String getContext(String key) {
  protected void scheduleInvocation() {
    OperationMeta operationMeta = restOperationMeta.getOperationMeta();

-    InvocationStartedEvent startedEvent = new InvocationStartedEvent(operationMeta.getMicroserviceQualifiedName(),
-        InvocationType.PRODUCER, System.nanoTime());
+    ProducerInvocationStartedEvent startedEvent = new ProducerInvocationStartedEvent(


Why do you introduce the ProducerInvocationStartedEvent?

WillemJiang · 2018-02-22T08:19:05Z

common/common-rest/src/main/java/org/apache/servicecomb/common/rest/AbstractRestInvocation.java

@@ -130,7 +129,7 @@ protected void scheduleInvocation() {
            return;
          }

-          runOnExecutor(startedEvent);
+          runOnExecutor(startedEvent.getStartedTime());


Using the Event could be easy for the user to extend.

WillemJiang · 2018-02-22T08:41:57Z

demo/perf/src/main/java/org/apache/servicecomb/demo/perf/PerfMetricsFilePublisher.java

          }
        }
      }
+    } catch (ServiceCombException ignored) {


Why did you ignored the exception.

WillemJiang · 2018-02-22T08:50:41Z

...n-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/publish/MetricsLoader.java

@@ -40,7 +42,7 @@ public MetricNode getMetricTree(String id, String... groupTagKeys) {
    if (metrics.containsKey(id)) {
      return new MetricNode(metrics.get(id), groupTagKeys);
    }
-    return null;
+    throw new ServiceCombException("no such id");


Please keep the id in the exception for the developer to find out the failed reason.

WillemJiang · 2018-02-22T08:53:32Z

...in/java/org/apache/servicecomb/metrics/core/event/servo/AbstractInvocationEventListener.java

+ * limitations under the License.
+ */
+
+package org.apache.servicecomb.metrics.core.event.servo;


Listener is application level code, it should not bind to the servo implementation.

WillemJiang · 2018-02-24T01:22:42Z

core/src/main/java/org/apache/servicecomb/core/Invocation.java

+  public void triggerStartExecutionEvent() {
+    if (InvocationType.PRODUCER.equals(invocationType)) {
+      this.startExecutionTime = System.nanoTime();
+      EventUtils.triggerEvent(new InvocationStartExecutionEvent(operationMeta.getMicroserviceQualifiedName()));


This EventUtils is more like a singleton.

OK , delete EventUtils and make EventBus as a singleton

WillemJiang · 2018-02-24T01:24:08Z

...ngmvc-client/src/main/java/org/apache/servicecomb/demo/springmvc/client/SpringmvcClient.java

      TestMgr.check(true, metrics.size() > 0);
      TestMgr.check(true, metrics.get(
-          "servicecomb.invocation(operation=springmvc.codeFirst.saySomething,role=producer,stage=whole,statistic=count,status=200)")
+          "servicecomb.invocation(operation=springmvc.codeFirst.saySomething,role=PRODUCER,stage=total,statistic=count,status=200)")


the PRODUCER is upper case, but other tags are lower case.

Done,add toLowerCase()

WillemJiang · 2018-02-24T01:25:46Z

demo/perf/src/main/java/org/apache/servicecomb/demo/perf/PerfMetricsFilePublisher.java

+    }
+    //before receive any request,there are no MetricsConst.SERVICECOMB_INVOCATION,so getMetricTree will throw ServiceCombException
+    catch (ServiceCombException ignored) {
+      return;


We need to log the exception for trace the issue

add a containsId method and no need throw exception any more

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

WillemJiang · 2018-02-24T05:58:33Z

...-metrics/src/test/java/org/apache/servicecomb/foundation/metrics/publish/TestMetricNode.java

+
+public class TestMetricNode {
+  @Test
+  public void test() {


The unit test method is not generic one, it should has some meaning which can help us to find out the key reason of failed test.

Ok ,add comments for explain check reason

You need to separate the test methods, please take https://github.com/apache/incubator-servicecomb-saga/blob/master/alpha/alpha-core/src/test/java/org/apache/servicecomb/saga/alpha/core/CompositeOmegaCallbackTest.java as an example.

WillemJiang · 2018-02-24T06:02:38Z

metrics/metrics-core/src/main/java/org/apache/servicecomb/metrics/core/MonitorManager.java

+    System.getProperties().setProperty("servo.pollers", time > 0 ? String.valueOf(time) : "5000");
+  }
+
+  public Counter getCounter(boolean isStepCounter, String name, String... tags) {


This method exposes the counter detail which is not right way.

separate into two method named getCounter and getStepCounter

WillemJiang · 2018-02-24T06:06:44Z

...s-core/src/main/java/org/apache/servicecomb/metrics/core/publish/HealthCheckerPublisher.java

  @CrossOrigin
-  public HealthCheckResult healthWithName(@PathVariable(name = "name") String name) {
-    return manager.check(name);
+  public Map<String, HealthCheckResult> checkHealthDetail() {


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

WillemJiang · 2018-02-24T14:41:45Z

...s-core/src/main/java/org/apache/servicecomb/metrics/core/publish/HealthCheckerPublisher.java

@@ -66,6 +47,6 @@ public boolean checkHealth() {
  @RequestMapping(path = "/detail", method = RequestMethod.GET)


The path name need to updated.

change to details

finnal path is /health/details

WillemJiang · 2018-02-24T14:44:31Z

...trics-core/src/test/java/org/apache/servicecomb/metrics/core/TestHealthCheckerPublisher.java

@@ -30,8 +30,7 @@

  @Test
  public void test() {
-    HealthCheckerManager manager = new HealthCheckerManager();
-    manager.register(new HealthChecker() {
+    HealthCheckerManager.getInstance().register(new HealthChecker() {


Here you just check one HealthChecker. The test should test at least zero，one and two HealthChecker.

zero，one and two HealthChecker check logic had added in TestHealthCheckerManager

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

liubao68 · 2018-02-26T02:18:37Z

...s-core/src/main/java/org/apache/servicecomb/metrics/core/publish/HealthCheckerPublisher.java

+@RequestMapping(path = "/health")
+public class HealthCheckerPublisher {
+  @RequestMapping(path = "/", method = RequestMethod.GET)
+  @CrossOrigin


This annotation may not work

zhengyangyong changed the title ~~[SCB-327] Update metrics publish data module~~ [SCB-327] Update metrics publish data module (Re-organized) Feb 12, 2018

WillemJiang reviewed Feb 14, 2018

View reviewed changes

zhengyangyong force-pushed the SCB-327-2 branch from ec11b9e to cdd41b8 Compare February 22, 2018 08:14

WillemJiang reviewed Feb 22, 2018

View reviewed changes

zhengyangyong force-pushed the SCB-327-2 branch 2 times, most recently from 195b5c2 to ede2c86 Compare February 23, 2018 12:09

WillemJiang reviewed Feb 24, 2018

View reviewed changes

zhengyangyong added 4 commits February 24, 2018 09:27

SCB-327 adjust metrics publish format to spring cloud netflix style

1836020

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

SCB-327 adjust health check package

e822269

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

SCB-327 remove spring dependency and use SPI mechanism

5376ae3

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

SCB-327 refactor metrics simplify monitors in order to improve implement

76b1df2

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

WillemJiang reviewed Feb 24, 2018

View reviewed changes

SCB-327 fix pr comment

db0755c

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

zhengyangyong force-pushed the SCB-327-2 branch from ede2c86 to db0755c Compare February 24, 2018 08:23

SCB-327 minor refactor and improve performance

7f85ba5

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

WillemJiang reviewed Feb 24, 2018

View reviewed changes

SCB-327 improve test case and fix pr comment

f7f3843

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

WillemJiang approved these changes Feb 25, 2018

View reviewed changes

liubao68 reviewed Feb 26, 2018

View reviewed changes

liubao68 approved these changes Feb 26, 2018

View reviewed changes

WillemJiang merged commit f55177e into apache:master Feb 26, 2018

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


		import org.apache.servicecomb.foundation.metrics.MetricsConst;

		public class MetricNode {


		public class ConsumerInvocationMonitor extends InvocationMonitor {
		public class ConsumerInvocationMonitor {

		@@ -66,6 +47,6 @@ public boolean checkHealth() {
		@RequestMapping(path = "/detail", method = RequestMethod.GET)

[SCB-327] Update metrics publish data module (Re-organized) #555

[SCB-327] Update metrics publish data module (Re-organized) #555

Conversation

zhengyangyong commented Feb 12, 2018 • edited

coveralls commented Feb 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhengyangyong commented Feb 12, 2018 •

edited

coveralls commented Feb 12, 2018 •

edited