[SCB-359]Servicecomb support latency statistic #1075

heyile · 2019-01-26T08:28:16Z

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.

coveralls · 2019-01-26T08:53:06Z

Coverage increased (+0.01%) to 85.928% when pulling 5c85242 on heyile:metricsInterval into d276f41 on apache:master.

coveralls · 2019-01-26T08:53:06Z

Coverage increased (+0.01%) to 85.462% when pulling e4c7ef2 on heyile:metricsInterval into df2002a on apache:master.

yhs0092 · 2019-01-28T03:05:39Z

Hi, maybe you can merge these three commit into one by git rebase : )

And I don't quite understand the meaning of your first commit, could you tell me?

heyile · 2019-01-28T07:14:18Z

Hi, maybe you can merge these three commit into one by git rebase : )

And I don't quite understand the meaning of your first commit, could you tell me?
the first commit is because that the name of variable may mislead others... and I want distinguish it from latency statistic. maybe rebase last two commits is better :-)

wujimin · 2019-01-28T13:21:00Z

...-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/meter/StatisticsBucket.java

+  @Override
+  public void calcMeasurements(List<Measurement> measurements, long msNow, long secondInterval) {
+    latencyIntervals.forEach(latencyInterval -> {
+      measurements.add(newMeasurement(rootId.withTag("buckets_interval", latencyInterval.toString()),


result of rootId.withTag("buckets_interval", latencyInterval.toString()) never changed, right?
save it to latencyInterval ?

wujimin · 2019-01-28T13:22:16Z

...-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/meter/StatisticsBucket.java

+  @Override
+  public void calcMeasurements(List<Measurement> measurements, long msNow, long secondInterval) {
+    latencyIntervals.forEach(latencyInterval -> {
+      measurements.add(newMeasurement(rootId.withTag("buckets_interval", latencyInterval.toString()),


change buckets_interval to bucket

maybe buckets_interval or buckets_latency is better ?

wujimin · 2019-01-28T13:25:56Z

...-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/meter/StatisticsBucket.java

+    if (arrays == null || arrays.length == 0) {
+      return results;
+    }
+    results.add(new LatencyInterval(0L, arrays[0] * MS_UNIT));


delete MS_UNIT
change to TimeUnit.MILLISECONDS.toNanos()

wujimin · 2019-01-28T13:29:18Z

...-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/meter/StatisticsBucket.java

+    return results;
+  }
+
+  public static String getStrFormatFromProperty(List<LatencyInterval> latencyIntervals) {


why place this method here?

I have moved it to the Class DefaultLogPublisher

wujimin · 2019-01-28T13:31:23Z

...-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/meter/StatisticsBucket.java

+    }
+
+    public long getAndUpdateLastTimes() {
+      long result = times.longValue() - lastUpdated;


save value of times.longValue() first
otherwise will lost some value.

...-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/meter/StatisticsBucket.java

wujimin · 2019-01-28T13:48:16Z

.../main/java/org/apache/servicecomb/metrics/core/meter/invocation/AbstractInvocationMeter.java

    totalTimer = creatStageTimer(MeterInvocationConst.STAGE_TOTAL);
    prepareTimer = creatStageTimer(MeterInvocationConst.STAGE_PREPARE);
    handlersRequestTimer = creatStageTimer(MeterInvocationConst.STAGE_HANDLERS_REQUEST);
    handlersResponseTimer = creatStageTimer(MeterInvocationConst.STAGE_HANDLERS_RESPONSE);
  }

+  protected StatisticsBucket creatStageBucket(String bucketsValue) {
+    return new StatisticsBucket(id.withTag(MeterInvocationConst.TAG_STAGE, bucketsValue));


latency bucket is a invocation stage? seems strange......

So. what value should I use here ?

wujimin · 2019-01-28T13:51:14Z

...rics-core/src/main/java/org/apache/servicecomb/metrics/core/publish/DefaultLogPublisher.java

@@ -60,27 +62,31 @@

  //sample
  private static final String SIMPLE_HEADER = "%s:\n  simple:\n"
-      + "    status          tps      latency             operation\n";
+      + "    status      tps    latency            %soperation\n";


two %s never changed, format them every time is not good
and difficult to understand.

wujimin · 2019-01-28T13:53:50Z

...rics-core/src/main/java/org/apache/servicecomb/metrics/core/publish/DefaultLogPublisher.java

@@ -89,7 +95,9 @@ public void init(GlobalRegistry globalRegistry, EventBus eventBus, MetricsBootst
        .get()) {
      return;
    }
-
+    latencyIntervals = StatisticsBucket


seems delete many empty lines between logic blocks?

... maybe it's doesn't matter ? Must I leave the spaces here ?

wujimin · 2019-01-28T14:02:53Z

...ics/metrics-core/src/main/java/org/apache/servicecomb/metrics/core/publish/PublishUtils.java

+    MeasurementNode timeInterval = stageNode.findChild(StatisticsBucket.TIME_INTERVAL);
+    // latency buckets
+    if (timeInterval != null) {
+      perfInfo.initInterval(timeInterval);


latency bucket is not belongs to perfInfo, right?

Yes. I move it to the class OperationPerf, as I thought that every operation has it's own latency

public class OperationPerf { private String operation; private Map<String, PerfInfo> stages = new HashMap<>(); private Map<String, BucketInfo> buckets = new HashMap<>();

wujimin · 2019-02-21T02:39:09Z

...-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/meter/StatisticsBucket.java

+import com.netflix.spectator.api.Id;
+import com.netflix.spectator.api.Measurement;
+
+public class StatisticsBucket extends AbstractPeriodMeter {


name is strange, seems can not express logic of this class?

wujimin · 2019-02-21T02:39:56Z

...-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/meter/StatisticsBucket.java

+    return results;
+  }
+
+  public static class LatencyInterval {


LatencyScope?
and comment that: [min, max)

wujimin · 2019-02-22T01:17:24Z

...-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/meter/StatisticsBucket.java

+    public LatencyInterval(long minValue, long maxValue, Id rootId) {
+      this.minValue = minValue;
+      this.maxValue = maxValue;
+      if (rootId != null) {


when will rootId be null?

wujimin · 2019-02-22T01:17:52Z

...-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/meter/StatisticsBucket.java

+  private List<LatencyInterval> latencyIntervals;
+
+  public StatisticsBucket(Id id) {
+    this.id = id;


why need both id and rootId?

wujimin · 2019-02-22T01:18:07Z

...-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/meter/StatisticsBucket.java

+      if (arrays == null || arrays.length == 0) {
+        return results;
+      }
+      results.add(new LatencyInterval(0L, arrays[0] * TimeUnit.MILLISECONDS.toNanos(1), rootId));


TimeUnit.MILLISECONDS.toNanos(arrays[0])

wujimin · 2019-02-22T01:19:02Z

...-metrics/src/main/java/org/apache/servicecomb/foundation/metrics/meter/StatisticsBucket.java

+
+    public String getStandardLatencyStr() {
+      StringBuilder sb = new StringBuilder();
+      sb.append("[").append(minValue / TimeUnit.MILLISECONDS.toNanos(1))


TimeUnit.NANOSECONDS.toMillis(minValue)

wujimin · 2019-03-06T08:04:57Z

...trics/src/main/java/org/apache/servicecomb/foundation/metrics/meter/BucketsDistribution.java

+import com.netflix.spectator.api.Id;
+import com.netflix.spectator.api.Measurement;
+
+public class BucketsDistribution extends AbstractPeriodMeter {


LatencyDistribution

wujimin · 2019-03-06T08:05:20Z

...trics/src/main/java/org/apache/servicecomb/foundation/metrics/meter/BucketsDistribution.java

+    return results;
+  }
+
+  public static class LatencyInterval {


LatencyScope

Ok, I will change the class name

wujimin · 2019-03-06T08:07:20Z

...trics/src/main/java/org/apache/servicecomb/foundation/metrics/meter/BucketsDistribution.java

+public class BucketsDistribution extends AbstractPeriodMeter {
+  private static final Logger LOGGER = LoggerFactory.getLogger(BucketsDistribution.class);
+
+  public static final String METRICS_LATENCY = DynamicPropertyFactory.getInstance()


default to ""

ok, change the default value is ""

wujimin · 2019-03-06T08:15:55Z

...trics/src/main/java/org/apache/servicecomb/foundation/metrics/meter/BucketsDistribution.java

+
+    public LatencyInterval(long minValue, long maxValue, Id rootId) {
+      this.minValue = TimeUnit.MILLISECONDS.toNanos(minValue);
+      this.maxValue = maxValue == Long.MAX_VALUE ? Long.MAX_VALUE : TimeUnit.MILLISECONDS.toNanos(maxValue);


MAX-VALUE -1 will overflow also.

I have redesign the method.

public static List<LatencyScope> parseLatencyIntervalListFromProperty(String property, Id rootId) { List<LatencyScope> results = new ArrayList<>(); try { String[] arrays = property.trim().split(","); if (arrays.length == 0) { return results; } results.add(new LatencyScope(0L, TimeUnit.MILLISECONDS.toNanos(Integer.parseInt(arrays[0])), createLatencyScopeId("0", arrays[0], rootId))); for (int i = 1; i < arrays.length; i++) { results.add(new LatencyScope(TimeUnit.MILLISECONDS.toNanos(Integer.parseInt(arrays[i - 1])), TimeUnit.MILLISECONDS.toNanos(Integer.parseInt(arrays[i])), createLatencyScopeId(arrays[i - 1], arrays[i], rootId))); } results.add(new LatencyScope( TimeUnit.MILLISECONDS.toNanos(Integer.parseInt(arrays[arrays.length - 1])), Long.MAX_VALUE, createLatencyScopeId(arrays[arrays.length - 1], " ", rootId))); } catch (Throwable e) { LOGGER.error("Failed to parse property servicecomb.metrics.invocation.latency.buckets", e); } return results; } /** * create LatencyScopeId with format [leftStr,rightStr) * @param leftStr leftValue * @param rightStr rightValue * @param rootId rootId * @return id */ private static Id createLatencyScopeId(String leftStr, String rightStr, Id rootId) { return rootId.withTag("statistic", LatencyScope.getStandardLatencyScopeStr(leftStr, rightStr, 0)); } public static class LatencyScope { private Id id; private LongAdder times = new LongAdder(); private long lastUpdated = 0L; private long minNanoSeconds; private long maxNanoSeconds; public LatencyScope(long minNanoSeconds, long maxNanoSeconds, Id id) throws Exception { if (minNanoSeconds >= maxNanoSeconds) { throw new Exception("maxNanoSeconds must greater than minNanoSeconds"); } this.minNanoSeconds = minNanoSeconds; this.maxNanoSeconds = maxNanoSeconds; this.id = id; }

wujimin · 2019-03-06T08:19:31Z

.../main/java/org/apache/servicecomb/metrics/core/meter/invocation/AbstractInvocationMeter.java

+  // bucket total
+  private BucketsDistribution totalBucket;
+  //stage
+  private Id stageId;


no need to cache stageId

I will removed it. and how about the name latencyDistribution

private LatencyDistributionMeter latencyDistribution; private long lastUpdated; public AbstractInvocationMeter(Registry registry, Id id) { this.registry = registry; this.id = id; latencyDistribution = createStatisticsBucket(MeterInvocationConst.TAG_LATENCY_DISTRIBUTION); totalTimer = createStageTimer(MeterInvocationConst.STAGE_TOTAL); prepareTimer = createStageTimer(MeterInvocationConst.STAGE_PREPARE); handlersRequestTimer = createStageTimer(MeterInvocationConst.STAGE_HANDLERS_REQUEST); handlersResponseTimer = createStageTimer(MeterInvocationConst.STAGE_HANDLERS_RESPONSE); }

wujimin · 2019-03-06T08:19:37Z

.../main/java/org/apache/servicecomb/metrics/core/meter/invocation/AbstractInvocationMeter.java

@@ -43,20 +44,30 @@
  // handler response
  private SimpleTimer handlersResponseTimer;

+  // bucket total
+  private BucketsDistribution totalBucket;


change name

I changed it to latencyDistribution

// latency distribution private LatencyDistributionMeter latencyDistribution;

wujimin · 2019-03-06T08:28:07Z

...rics-core/src/main/java/org/apache/servicecomb/metrics/core/publish/DefaultLogPublisher.java

+      + "        sFiltersResp: %-18s sendResp    : %s\n";
+
+
+  private List<String> sortedLatencyIntervals;


comments these fields with sample configuration

wujimin · 2019-03-06T08:37:34Z

...trics/src/main/java/org/apache/servicecomb/foundation/metrics/meter/BucketsDistribution.java

+
+  public static class LatencyInterval {
+
+    public static final int leastLength = 7;


only for publish, not relate to calc......

I have moved the leastLength to the class DefaultLogPublisher.
as I thought that this number is relate to the log and has no relation with tag

public class DefaultLogPublisher implements MetricsInitializer { private static final int LEAST_LATENCY_SCOPE_STR_LENGTH = 7;

wujimin · 2019-03-06T08:38:47Z

...trics/src/main/java/org/apache/servicecomb/foundation/metrics/meter/BucketsDistribution.java

+    public LatencyInterval(long minValue, long maxValue, Id rootId) {
+      this.minValue = TimeUnit.MILLISECONDS.toNanos(minValue);
+      this.maxValue = maxValue == Long.MAX_VALUE ? Long.MAX_VALUE : TimeUnit.MILLISECONDS.toNanos(maxValue);
+      this.id = rootId.withTag("statistic", getStandardLatencyStr(minValue, maxValue, leastLength));


tag no need to same to output format.....
tag is "[0,1)", and output format is "[0,1) ", seems not conflict, right?

I think so, I will redesign the method

private static Id createLatencyScopeId(String leftStr, String rightStr, Id rootId) { return rootId.withTag("statistic", LatencyScope.getStandardLatencyScopeStr(leftStr, rightStr, 0)); } public static String getStandardLatencyScopeStr(String leftStr, String rightStr, int minLength) { StringBuilder sb = new StringBuilder(); sb.append("[").append(leftStr) .append(",") .append(rightStr) .append(")"); if (sb.length() < minLength) { for (int i = 0; i < minLength - sb.length(); i++) { sb.append(' '); } } return sb.toString(); }

wujimin · 2019-03-06T08:49:58Z

...rics-core/src/main/java/org/apache/servicecomb/metrics/core/publish/DefaultLogPublisher.java

+    return String.format(intervalFormat, array);
+  }
+
+  public String getLogStrFormat(List<String> latencyIntervals) {


1.private
2.change name relate to latency distribution

done. the new method is as bellow

/** * <p> * get respond str format from list of str. for example * {@code * List<String> strList = Arrays.asList("[0,1) ", "[1,4) ", "[4, ) "); * String strFormat = getLatencyScopeFormat(strList); * System.out.println(String.format(strFormat,strList.toArray())); * System.out.println(String.format(strFormat, 1, 23, 433)); * System.out.println(String.format(strFormat, 121, 2, 4)); * } * In the example above, result is showed as bellow: * [0,1) [1,4) [4, ) * 1 23 433 * 121 2 4 * </p> * @param latencyIntervals list of str * @return str format */ private String getLatencyScopeFormat(List<String> latencyIntervals) {

wujimin · 2019-03-06T09:21:39Z

...g/apache/servicecomb/metrics/core/publish/model/invocation/OperationLatencyDistribution.java

+
+import com.google.common.annotations.VisibleForTesting;
+
+public class OperationLatencyDistribution {


try to delete this class, make logic simpler

I have replace the class with

public class OperationPerf { private String operation; private Map<String, PerfInfo> stages = new HashMap<>(); private List<Integer> latencyDistribution = new ArrayList<>();

wujimin · 2019-03-07T00:58:50Z

...rics-core/src/main/java/org/apache/servicecomb/metrics/core/publish/DefaultLogPublisher.java

+   * @param latencyIntervals list of str
+   * @return str format
+   */
+  private String getLatencyScopeFormat(List<String> latencyIntervals) {


change parameter name

… code

heyile closed this Jan 26, 2019

heyile reopened this Jan 26, 2019

heyile changed the title ~~Metrics interval~~ [SCB-359]Servicecomb support latency statistic Jan 26, 2019

wujimin reviewed Jan 28, 2019

View reviewed changes

wujimin reviewed Feb 22, 2019

View reviewed changes

wujimin reviewed Mar 6, 2019

View reviewed changes

wujimin reviewed Mar 7, 2019

View reviewed changes

heyile added 3 commits March 7, 2019 21:09

[SCB-1046] file upload support file array for the same name: optimize…

6d3f92a

… code

[SCB-359]Servicecomb support latency statistic

e89f4ea

[SCB-359]Servicecomb support latency statistic: fixed details

e4c7ef2

wujimin approved these changes Mar 7, 2019

View reviewed changes

wujimin merged commit f4ae766 into apache:master Mar 7, 2019

		+ " sFiltersResp: %-18s sendResp : %s\n";


		private List<String> sortedLatencyIntervals;


		public static class LatencyInterval {

		public static final int leastLength = 7;


		import com.google.common.annotations.VisibleForTesting;

		public class OperationLatencyDistribution {

[SCB-359]Servicecomb support latency statistic #1075

[SCB-359]Servicecomb support latency statistic #1075

Conversation

heyile commented Jan 26, 2019

coveralls commented Jan 26, 2019

coveralls commented Jan 26, 2019 • edited

yhs0092 commented Jan 28, 2019

heyile commented Jan 28, 2019

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

heyile Feb 22, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heyile Feb 22, 2019 • 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

heyile Mar 6, 2019 • 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

heyile Mar 6, 2019 • 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

coveralls commented Jan 26, 2019 •

edited

heyile Feb 22, 2019 •

edited

heyile Feb 22, 2019 •

edited

heyile Mar 6, 2019 •

edited

heyile Mar 6, 2019 •

edited