[GitHub] WillemJiang commented on a change in pull request #555: [SCB-327] Update metrics publish data module (Re-organized)

2018-02-24 Thread GitBox
WillemJiang commented on a change in pull request #555: [SCB-327] Update 
metrics publish data module (Re-organized)
URL: 
https://github.com/apache/incubator-servicecomb-java-chassis/pull/555#discussion_r170422863
 
 

 ##
 File path: 
metrics/metrics-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() {
 
 Review comment:
   Here you just check one HealthChecker. The test should test at least 
zero?one and two HealthChecker. 


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] WillemJiang commented on a change in pull request #555: [SCB-327] Update metrics publish data module (Re-organized)

2018-02-24 Thread GitBox
WillemJiang commented on a change in pull request #555: [SCB-327] Update 
metrics publish data module (Re-organized)
URL: 
https://github.com/apache/incubator-servicecomb-java-chassis/pull/555#discussion_r170422785
 
 

 ##
 File path: 
metrics/metrics-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)
 
 Review comment:
   The path name need to updated.


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] WillemJiang commented on a change in pull request #555: [SCB-327] Update metrics publish data module (Re-organized)

2018-02-24 Thread GitBox
WillemJiang commented on a change in pull request #555: [SCB-327] Update 
metrics publish data module (Re-organized)
URL: 
https://github.com/apache/incubator-servicecomb-java-chassis/pull/555#discussion_r170422650
 
 

 ##
 File path: 
foundations/foundation-metrics/src/test/java/org/apache/servicecomb/foundation/metrics/publish/TestMetricNode.java
 ##
 @@ -0,0 +1,65 @@
+/*
+ * 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 java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+public class TestMetricNode {
+  @Test
+  public void test() {
 
 Review comment:
   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.


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] WillemJiang commented on a change in pull request #555: [SCB-327] Update metrics publish data module (Re-organized)

2018-02-23 Thread GitBox
WillemJiang commented on a change in pull request #555: [SCB-327] Update 
metrics publish data module (Re-organized)
URL: 
https://github.com/apache/incubator-servicecomb-java-chassis/pull/555#discussion_r170412777
 
 

 ##
 File path: 
metrics/metrics-core/src/main/java/org/apache/servicecomb/metrics/core/publish/HealthCheckerPublisher.java
 ##
 @@ -60,16 +53,19 @@ private void init(HealthCheckerManager manager) {
 
   @RequestMapping(path = "/", method = RequestMethod.GET)
   @CrossOrigin
-  public Map health() {
-return manager.check();
+  public boolean checkHealth() {
+Map results = manager.check();
+for (HealthCheckResult result : results.values()) {
+  if (!result.isHealthy()) {
+return false;
+  }
+}
+return true;
   }
 
-  @ApiResponses({
-  @ApiResponse(code = 400, response = String.class, message = "illegal 
request content"),
-  })
-  @RequestMapping(path = "/{name}", method = RequestMethod.GET)
+  @RequestMapping(path = "/detail", method = RequestMethod.GET)
   @CrossOrigin
-  public HealthCheckResult healthWithName(@PathVariable(name = "name") String 
name) {
-return manager.check(name);
+  public Map checkHealthDetail() {
 
 Review comment:
   Details


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] WillemJiang commented on a change in pull request #555: [SCB-327] Update metrics publish data module (Re-organized)

2018-02-23 Thread GitBox
WillemJiang commented on a change in pull request #555: [SCB-327] Update 
metrics publish data module (Re-organized)
URL: 
https://github.com/apache/incubator-servicecomb-java-chassis/pull/555#discussion_r170412685
 
 

 ##
 File path: 
metrics/metrics-core/src/main/java/org/apache/servicecomb/metrics/core/MonitorManager.java
 ##
 @@ -0,0 +1,181 @@
+/*
+ * 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.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;
+import java.util.TreeMap;
+import java.util.concurrent.Callable;
+
+import org.apache.servicecomb.foundation.common.concurrent.ConcurrentHashMapEx;
+import org.apache.servicecomb.foundation.metrics.MetricsConst;
+
+import com.netflix.config.DynamicPropertyFactory;
+import com.netflix.servo.BasicMonitorRegistry;
+import com.netflix.servo.MonitorRegistry;
+import com.netflix.servo.monitor.BasicCounter;
+import com.netflix.servo.monitor.BasicGauge;
+import com.netflix.servo.monitor.BasicTimer;
+import com.netflix.servo.monitor.Counter;
+import com.netflix.servo.monitor.Gauge;
+import com.netflix.servo.monitor.MaxGauge;
+import com.netflix.servo.monitor.Monitor;
+import com.netflix.servo.monitor.MonitorConfig;
+import com.netflix.servo.monitor.MonitorConfig.Builder;
+import com.netflix.servo.monitor.StepCounter;
+import com.netflix.servo.monitor.Timer;
+import com.netflix.servo.tag.Tag;
+import com.netflix.servo.tag.TagList;
+
+public class MonitorManager {
+
+  private final Map counters;
+
+  private final Map maxGauges;
+
+  private final Map gauges;
+
+  private final Map timers;
+
+  private final MonitorRegistry basicMonitorRegistry;
+
+  private static final MonitorManager INSTANCE = new MonitorManager();
+
+  public static MonitorManager getInstance() {
+return INSTANCE;
+  }
+
+  private MonitorManager() {
+this.counters = new ConcurrentHashMapEx<>();
+this.maxGauges = new ConcurrentHashMapEx<>();
+this.gauges = new ConcurrentHashMapEx<>();
+this.timers = new ConcurrentHashMapEx<>();
+this.basicMonitorRegistry = new BasicMonitorRegistry();
+setupWindowTime();
+registerSystemMetrics();
+  }
+
+  private void setupWindowTime() {
+int time = 
DynamicPropertyFactory.getInstance().getIntProperty(MetricsConfig.METRICS_WINDOW_TIME,
 5000).get();
+System.getProperties().setProperty("servo.pollers", time > 0 ? 
String.valueOf(time) : "5000");
+  }
+
+  public Counter getCounter(boolean isStepCounter, String name, String... 
tags) {
 
 Review comment:
   This method exposes the counter detail which is not right way.


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] WillemJiang commented on a change in pull request #555: [SCB-327] Update metrics publish data module (Re-organized)

2018-02-23 Thread GitBox
WillemJiang commented on a change in pull request #555: [SCB-327] Update 
metrics publish data module (Re-organized)
URL: 
https://github.com/apache/incubator-servicecomb-java-chassis/pull/555#discussion_r170412622
 
 

 ##
 File path: 
foundations/foundation-metrics/src/test/java/org/apache/servicecomb/foundation/metrics/publish/TestMetricNode.java
 ##
 @@ -0,0 +1,65 @@
+/*
+ * 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 java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+public class TestMetricNode {
+  @Test
+  public void test() {
 
 Review comment:
   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.


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] WillemJiang commented on a change in pull request #555: [SCB-327] Update metrics publish data module (Re-organized)

2018-02-23 Thread GitBox
WillemJiang commented on a change in pull request #555: [SCB-327] Update 
metrics publish data module (Re-organized)
URL: 
https://github.com/apache/incubator-servicecomb-java-chassis/pull/555#discussion_r170404621
 
 

 ##
 File path: 
demo/perf/src/main/java/org/apache/servicecomb/demo/perf/PerfMetricsFilePublisher.java
 ##
 @@ -75,51 +70,57 @@ private void collectVertxMetrics(MetricsLoader loader, 
StringBuilder sb) {
   }
 
   private void collectMetrics(MetricsLoader loader, StringBuilder sb) {
-MetricNode treeNode = loader
-.getMetricTree(MetricsConst.SERVICECOMB_INVOCATION, 
MetricsConst.TAG_ROLE, MetricsConst.TAG_OPERATION,
-MetricsConst.TAG_STATUS);
+MetricNode treeNode;
+try {
+  treeNode = loader
+  .getMetricTree(MetricsConst.SERVICECOMB_INVOCATION, 
MetricsConst.TAG_ROLE, MetricsConst.TAG_OPERATION,
+  MetricsConst.TAG_STATUS);
+}
+//before receive any request,there are no 
MetricsConst.SERVICECOMB_INVOCATION,so getMetricTree will throw 
ServiceCombException
+catch (ServiceCombException ignored) {
+  return;
 
 Review comment:
   We need to log the exception for trace the issue


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] WillemJiang commented on a change in pull request #555: [SCB-327] Update metrics publish data module (Re-organized)

2018-02-23 Thread GitBox
WillemJiang commented on a change in pull request #555: [SCB-327] Update 
metrics publish data module (Re-organized)
URL: 
https://github.com/apache/incubator-servicecomb-java-chassis/pull/555#discussion_r170404504
 
 

 ##
 File path: 
demo/demo-springmvc/springmvc-client/src/main/java/org/apache/servicecomb/demo/springmvc/client/SpringmvcClient.java
 ##
 @@ -87,10 +87,10 @@ public static void run() {
   Map metrics = restTemplate.getForObject(prefix + 
"/metrics", Map.class);
 
   TestMgr
-  .check(true, metrics.get("jvm(statistic=gauge,name=heapUsed)") != 0);
+  .check(true, metrics.get("jvm(name=heapUsed,statistic=gauge)") != 0);
   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)")
 
 Review comment:
   the PRODUCER is upper case, but other tags are lower case.


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] WillemJiang commented on a change in pull request #555: [SCB-327] Update metrics publish data module (Re-organized)

2018-02-23 Thread GitBox
WillemJiang commented on a change in pull request #555: [SCB-327] Update 
metrics publish data module (Re-organized)
URL: 
https://github.com/apache/incubator-servicecomb-java-chassis/pull/555#discussion_r170404398
 
 

 ##
 File path: core/src/main/java/org/apache/servicecomb/core/Invocation.java
 ##
 @@ -185,18 +185,18 @@ public String getMicroserviceQualifiedName() {
 return operationMeta.getMicroserviceQualifiedName();
   }
 
-  public void triggerStartProcessingEvent() {
-this.startProcessingTime = System.nanoTime();
-EventUtils.triggerEvent(new InvocationStartProcessingEvent(
-operationMeta.getMicroserviceQualifiedName(), this.invocationType));
+  public void triggerStartExecutionEvent() {
+if (InvocationType.PRODUCER.equals(invocationType)) {
+  this.startExecutionTime = System.nanoTime();
+  EventUtils.triggerEvent(new 
InvocationStartExecutionEvent(operationMeta.getMicroserviceQualifiedName()));
 
 Review comment:
   This EventUtils is more like a singleton.


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] WillemJiang commented on a change in pull request #555: [SCB-327] Update metrics publish data module (Re-organized)

2018-02-22 Thread GitBox
WillemJiang commented on a change in pull request #555: [SCB-327] Update 
metrics publish data module (Re-organized)
URL: 
https://github.com/apache/incubator-servicecomb-java-chassis/pull/555#discussion_r169888260
 
 

 ##
 File path: 
metrics/metrics-core/src/main/java/org/apache/servicecomb/metrics/core/event/servo/AbstractInvocationEventListener.java
 ##
 @@ -0,0 +1,35 @@
+/*
+ * 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.metrics.core.event.servo;
 
 Review comment:
   Listener is application level code, it should not bind to the servo 
implementation. 


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] WillemJiang commented on a change in pull request #555: [SCB-327] Update metrics publish data module (Re-organized)

2018-02-22 Thread GitBox
WillemJiang commented on a change in pull request #555: [SCB-327] Update 
metrics publish data module (Re-organized)
URL: 
https://github.com/apache/incubator-servicecomb-java-chassis/pull/555#discussion_r169881296
 
 

 ##
 File path: 
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());
 
 Review comment:
   Using the Event could  be easy for the user to extend.


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] WillemJiang commented on a change in pull request #555: [SCB-327] Update metrics publish data module (Re-organized)

2018-02-22 Thread GitBox
WillemJiang commented on a change in pull request #555: [SCB-327] Update 
metrics publish data module (Re-organized)
URL: 
https://github.com/apache/incubator-servicecomb-java-chassis/pull/555#discussion_r169881157
 
 

 ##
 File path: 
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(
 
 Review comment:
   Why do you introduce the ProducerInvocationStartedEvent? 


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] WillemJiang commented on a change in pull request #555: [SCB-327] Update metrics publish data module (Re-organized)

2018-02-22 Thread GitBox
WillemJiang commented on a change in pull request #555: [SCB-327] Update 
metrics publish data module (Re-organized)
URL: 
https://github.com/apache/incubator-servicecomb-java-chassis/pull/555#discussion_r169885809
 
 

 ##
 File path: 
demo/perf/src/main/java/org/apache/servicecomb/demo/perf/PerfMetricsFilePublisher.java
 ##
 @@ -75,54 +76,56 @@ private void collectVertxMetrics(MetricsLoader loader, 
StringBuilder sb) {
   }
 
   private void collectMetrics(MetricsLoader loader, StringBuilder sb) {
-MetricNode treeNode = loader
-.getMetricTree(MetricsConst.SERVICECOMB_INVOCATION, 
MetricsConst.TAG_ROLE, MetricsConst.TAG_OPERATION,
-MetricsConst.TAG_STATUS);
-
-if (treeNode != null && treeNode.getChildren().size() != 0) {
-  MetricNode consumerNode = 
treeNode.getChildren().get(MetricsConst.ROLE_CONSUMER);
-  if (consumerNode != null) {
-sb.append("consumer:\n");
-sb.append("  tps latency(ms) status  operation\n");
-for (Entry operationNode : 
consumerNode.getChildren().entrySet()) {
-  for (Entry statusNode : 
operationNode.getValue().getChildren().entrySet()) {
-sb.append(String.format("  %-7.0f %-11.3f %-9s %s\n",
-statusNode.getValue()
-
.getFirstMatchMetricValue(Lists.newArrayList(MetricsConst.TAG_STAGE, 
MetricsConst.TAG_STATISTIC),
-Lists.newArrayList(MetricsConst.STAGE_WHOLE, "tps")),
-statusNode.getValue()
-
.getFirstMatchMetricValue(Lists.newArrayList(MetricsConst.TAG_STAGE, 
MetricsConst.TAG_STATISTIC),
-Lists.newArrayList(MetricsConst.STAGE_WHOLE, 
"latency")),
-statusNode.getKey(),
-operationNode.getKey()));
+try {
+  MetricNode treeNode = loader
+  .getMetricTree(MetricsConst.SERVICECOMB_INVOCATION, 
MetricsConst.TAG_ROLE, MetricsConst.TAG_OPERATION,
+  MetricsConst.TAG_STATUS);
+
+  if (treeNode != null && treeNode.getChildren().size() != 0) {
+MetricNode consumerNode = 
treeNode.getChildren().get(String.valueOf(InvocationType.CONSUMER));
+if (consumerNode != null) {
+  sb.append("consumer:\n");
+  sb.append("  tps latency(ms) status  operation\n");
+  for (Entry operationNode : 
consumerNode.getChildren().entrySet()) {
+for (Entry statusNode : 
operationNode.getValue().getChildren().entrySet()) {
+  sb.append(String.format("  %-7.0f %-11.3f %-9s %s\n",
+  statusNode.getValue()
+  .getFirstMatchMetricValue(MetricsConst.TAG_STAGE, 
MetricsConst.STAGE_TOTAL,
+  MetricsConst.TAG_STATISTIC, "tps"),
+  statusNode.getValue()
+  .getFirstMatchMetricValue(TimeUnit.MILLISECONDS, 
MetricsConst.TAG_STAGE, MetricsConst.STAGE_TOTAL,
+  MetricsConst.TAG_STATISTIC, "latency"),
+  statusNode.getKey(), operationNode.getKey()));
+}
   }
 }
-  }
 
-  MetricNode producerNode = 
treeNode.getChildren().get(MetricsConst.ROLE_PRODUCER);
-  if (producerNode != null) {
-sb.append("producer:\n");
-sb.append("  tps latency(ms) queue(ms) execute(ms) status  
operation\n");
-for (Entry operationNode : 
producerNode.getChildren().entrySet()) {
-  for (Entry statusNode : 
operationNode.getValue().getChildren().entrySet()) {
-sb.append(String.format("  %-7.0f %-11.3f %-9.3f %-11.3f %-7s 
%s\n",
-statusNode.getValue()
-
.getFirstMatchMetricValue(Lists.newArrayList(MetricsConst.TAG_STAGE, 
MetricsConst.TAG_STATISTIC),
-Lists.newArrayList(MetricsConst.STAGE_WHOLE, "tps")),
-statusNode.getValue()
-
.getFirstMatchMetricValue(Lists.newArrayList(MetricsConst.TAG_STAGE, 
MetricsConst.TAG_STATISTIC),
-Lists.newArrayList(MetricsConst.STAGE_WHOLE, 
"latency")),
-statusNode.getValue()
-
.getFirstMatchMetricValue(Lists.newArrayList(MetricsConst.TAG_STAGE, 
MetricsConst.TAG_STATISTIC),
-Lists.newArrayList(MetricsConst.STAGE_QUEUE, 
"latency")),
-statusNode.getValue()
-
.getFirstMatchMetricValue(Lists.newArrayList(MetricsConst.TAG_STAGE, 
MetricsConst.TAG_STATISTIC),
-Lists.newArrayList(MetricsConst.STAGE_EXECUTION, 
"latency")),
-statusNode.getKey(),
-operationNode.getKey()));
+MetricNode producerNode = 
treeNode.getChildren().get(String.valueOf(InvocationType.PRODUCER));
+if (producerNode != null) {
+  sb.append("producer:\n");
+  sb.append("  tps 

[GitHub] WillemJiang commented on a change in pull request #555: [SCB-327] Update metrics publish data module (Re-organized)

2018-02-22 Thread GitBox
WillemJiang commented on a change in pull request #555: [SCB-327] Update 
metrics publish data module (Re-organized)
URL: 
https://github.com/apache/incubator-servicecomb-java-chassis/pull/555#discussion_r169887598
 
 

 ##
 File path: 
foundations/foundation-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");
 
 Review comment:
   Please keep the id in the exception for the developer to find out the failed 
reason.


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] WillemJiang commented on a change in pull request #555: [SCB-327] Update metrics publish data module (Re-organized)

2018-02-13 Thread GitBox
WillemJiang commented on a change in pull request #555: [SCB-327] Update 
metrics publish data module (Re-organized)
URL: 
https://github.com/apache/incubator-servicecomb-java-chassis/pull/555#discussion_r168063945
 
 

 ##
 File path: 
metrics/metrics-core/src/main/java/org/apache/servicecomb/metrics/core/monitor/RegistryMonitor.java
 ##
 @@ -51,16 +48,14 @@ public ProducerInvocationMonitor 
getProducerInvocationMonitor(String operationNa
 return producerInvocationMonitors.computeIfAbsent(operationName, i -> new 
ProducerInvocationMonitor(operationName));
   }
 
-  public RegistryMetric toRegistryMetric(int windowTimeIndex) {
-Map consumerInvocationMetrics = new 
HashMap<>();
+  public Map measure(int windowTimeIndex, boolean 
calculateLatency) {
+Map measurements = new HashMap<>(systemMonitor.measure());
 
 Review comment:
   If we want measure the invocation monitor, we don't need to know if it 
consumerInvocationMonitor or producerInvocationMonitor.


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] WillemJiang commented on a change in pull request #555: [SCB-327] Update metrics publish data module (Re-organized)

2018-02-13 Thread GitBox
WillemJiang commented on a change in pull request #555: [SCB-327] Update 
metrics publish data module (Re-organized)
URL: 
https://github.com/apache/incubator-servicecomb-java-chassis/pull/555#discussion_r168063658
 
 

 ##
 File path: 
metrics/metrics-core/src/main/java/org/apache/servicecomb/metrics/core/monitor/ConsumerInvocationMonitor.java
 ##
 @@ -17,11 +17,12 @@
 
 package org.apache.servicecomb.metrics.core.monitor;
 
+import java.util.HashMap;
+import java.util.Map;
 
-import org.apache.servicecomb.metrics.common.ConsumerInvocationMetric;
-import org.apache.servicecomb.metrics.common.MetricsConst;
+import org.apache.servicecomb.foundation.metrics.MetricsConst;
 
-public class ConsumerInvocationMonitor extends InvocationMonitor {
+public class ConsumerInvocationMonitor {
 
 Review comment:
   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.
   


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] WillemJiang commented on a change in pull request #555: [SCB-327] Update metrics publish data module (Re-organized)

2018-02-13 Thread GitBox
WillemJiang commented on a change in pull request #555: [SCB-327] Update 
metrics publish data module (Re-organized)
URL: 
https://github.com/apache/incubator-servicecomb-java-chassis/pull/555#discussion_r168062197
 
 

 ##
 File path: 
metrics/metrics-core/src/main/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());
 
 Review comment:
   Why did you remove this line?


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] WillemJiang commented on a change in pull request #555: [SCB-327] Update metrics publish data module (Re-organized)

2018-02-13 Thread GitBox
WillemJiang commented on a change in pull request #555: [SCB-327] Update 
metrics publish data module (Re-organized)
URL: 
https://github.com/apache/incubator-servicecomb-java-chassis/pull/555#discussion_r168060887
 
 

 ##
 File path: 
foundations/foundation-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/publish/MetricsLoader.java
 ##
 @@ -0,0 +1,56 @@
+/*
+ * 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 java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+
+//load origin metrics value and publish tree
+public class MetricsLoader {
+
+  private final Map metrics;
+
+  public MetricsLoader(Map metrics) {
+this.metrics = new HashMap<>();
+for (Entry entry : metrics.entrySet()) {
+  Metric metric = new Metric(entry.getKey(), entry.getValue());
+  this.metrics.computeIfAbsent(metric.getName(), m -> new 
ArrayList<>()).add(metric);
+}
+  }
+
+  public MetricNode getMetricTree(String id, String... groupTagKeys) {
+if (metrics.containsKey(id)) {
+  return new MetricNode(metrics.get(id), groupTagKeys);
+}
+return null;
 
 Review comment:
   I'm not a big fan of null, you can throw exception or using Option return 
the value.


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] WillemJiang commented on a change in pull request #555: [SCB-327] Update metrics publish data module (Re-organized)

2018-02-13 Thread GitBox
WillemJiang commented on a change in pull request #555: [SCB-327] Update 
metrics publish data module (Re-organized)
URL: 
https://github.com/apache/incubator-servicecomb-java-chassis/pull/555#discussion_r168060645
 
 

 ##
 File path: 
foundations/foundation-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/publish/MetricNode.java
 ##
 @@ -0,0 +1,108 @@
+/*
+ * 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 java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+
+import org.apache.servicecomb.foundation.metrics.MetricsConst;
+
+public class MetricNode {
 
 Review comment:
   Can you show me the unit test of MetricNode? 


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] WillemJiang commented on a change in pull request #555: [SCB-327] Update metrics publish data module (Re-organized)

2018-02-13 Thread GitBox
WillemJiang commented on a change in pull request #555: [SCB-327] Update 
metrics publish data module (Re-organized)
URL: 
https://github.com/apache/incubator-servicecomb-java-chassis/pull/555#discussion_r168059325
 
 

 ##
 File path: 
foundations/foundation-metrics/src/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 {
 
 Review comment:
   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.


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:
us...@infra.apache.org


With regards,
Apache Git Services


[GitHub] WillemJiang commented on a change in pull request #555: [SCB-327] Update metrics publish data module (Re-organized)

2018-02-13 Thread GitBox
WillemJiang commented on a change in pull request #555: [SCB-327] Update 
metrics publish data module (Re-organized)
URL: 
https://github.com/apache/incubator-servicecomb-java-chassis/pull/555#discussion_r168057392
 
 

 ##
 File path: core/src/main/java/org/apache/servicecomb/core/Invocation.java
 ##
 @@ -188,15 +188,16 @@ public String getMicroserviceQualifiedName() {
   public void triggerStartProcessingEvent() {
 this.startProcessingTime = System.nanoTime();
 EventUtils.triggerEvent(new InvocationStartProcessingEvent(
-operationMeta.getMicroserviceQualifiedName(), this.invocationType, 
startProcessingTime - startTime));
+operationMeta.getMicroserviceQualifiedName(), this.invocationType));
   }
 
-  public void triggerFinishedEvent(int statusCode, boolean success) {
+  public void triggerFinishedEvent(int statusCode) {
 long finishedTime = System.nanoTime();
 EventUtils
-.triggerEvent(new 
InvocationFinishedEvent(operationMeta.getMicroserviceQualifiedName(),
-this.invocationType, finishedTime - startProcessingTime,
-finishedTime - startTime, statusCode, success));
+.triggerEvent(new 
InvocationFinishedEvent(operationMeta.getMicroserviceQualifiedName(), 
this.invocationType,
+startProcessingTime - startTime,
 
 Review comment:
   Can you explain the meaning of startTime, startProcessingTime?
   


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:
us...@infra.apache.org


With regards,
Apache Git Services