You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by yew1eb <gi...@git.apache.org> on 2017/10/14 10:50:49 UTC
[GitHub] flink pull request #4826: [FLINK-7608][metric] Refactoring latency statistic...
GitHub user yew1eb opened a pull request:
https://github.com/apache/flink/pull/4826
[FLINK-7608][metric] Refactoring latency statistics metric
A detailed description of this PR, see [#issue FLINK-7608: LatencyGauge change to histogram metric](https://issues.apache.org/jira/browse/FLINK-7608)
## Verifying this change
This change is already covered by existing tests.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (no)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no)
- The serializers: (no)
- The runtime per-record code paths (performance sensitive): (no)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)
## Documentation
- Does this pull request introduce a new feature? (no)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/yew1eb/flink FLINK-7608
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/4826.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #4826
----
commit 890616b828d979eed336d546300f442ac7f75b09
Author: yew1eb <ye...@gmail.com>
Date: 2017-10-14T09:19:49Z
refactoring latency statistics metric
----
---
[GitHub] flink issue #4826: [FLINK-7608][metric] Refactor latency statistics metric
Posted by yew1eb <gi...@git.apache.org>.
Github user yew1eb commented on the issue:
https://github.com/apache/flink/pull/4826
Sorry to reply late, @zentol could you will merge this PR?
---
[GitHub] flink pull request #4826: [FLINK-7608][metric] Refactor latency statistics m...
Posted by yew1eb <gi...@git.apache.org>.
Github user yew1eb closed the pull request at:
https://github.com/apache/flink/pull/4826
---
[GitHub] flink pull request #4826: [FLINK-7608][metric] Refactoring latency statistic...
Posted by yew1eb <gi...@git.apache.org>.
Github user yew1eb commented on a diff in the pull request:
https://github.com/apache/flink/pull/4826#discussion_r145293108
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/util/latency/LatencyHistogram.java ---
@@ -0,0 +1,51 @@
+/*
+ * 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.flink.streaming.util.latency;
+
+import org.apache.flink.metrics.Histogram;
+import org.apache.flink.metrics.HistogramStatistics;
+
+import org.apache.commons.math3.stat.descriptive.DescriptiveStatistics;
+
+/**
+ * The {@link LatencyHistogram} use a DescriptiveStatistics {@link DescriptiveStatistics} as a Flink {@link Histogram}.
+ */
+public class LatencyHistogram implements org.apache.flink.metrics.Histogram {
+
+ private final DescriptiveStatistics latencyHistogram;
+
+ public LatencyHistogram(int windowSize) {
+ // 512 element window (4 kb)
--- End diff --
will remove.
---
[GitHub] flink pull request #4826: [FLINK-7608][metric] Refactoring latency statistic...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4826#discussion_r145114263
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/util/latency/LatencyStats.java ---
@@ -0,0 +1,67 @@
+/*
+ * 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.flink.streaming.util.latency;
+
+import org.apache.flink.metrics.MetricGroup;
+import org.apache.flink.streaming.runtime.streamrecord.LatencyMarker;
+
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * The {@link LatencyStats} objects are used to track and report on the behavior of latencies across measurements.
+ */
+public class LatencyStats {
+ private final Map<String, LatencyHistogram> latencyStats = new HashMap<>();
+ private final MetricGroup metricGroup;
+ private final int historySize;
+
+ public LatencyStats(MetricGroup metricGroup, int historySize) {
+ this.metricGroup = metricGroup;
+ this.historySize = historySize;
+ }
+
+ public void reportLatency(LatencyMarker marker, boolean isSink) {
+ String latencyMetricName = identifyLatencySource(marker, !isSink);
+ LatencyHistogram latencyHistogram = this.latencyStats.get(latencyMetricName);
+ if (latencyHistogram == null) {
+ latencyHistogram = new LatencyHistogram(this.historySize);
+ this.latencyStats.put(latencyMetricName, latencyHistogram);
+ this.metricGroup.histogram(latencyMetricName, latencyHistogram);
+ }
+
+ long now = System.currentTimeMillis();
+ latencyHistogram.update(now - marker.getMarkedTime());
+ }
+
+ /**
+ * Creates a Identifier for a latency source. from a given {@code LatencyMarker}.
--- End diff --
"an identifier"
---
[GitHub] flink issue #4826: [FLINK-7608][metric] Refactoring latency statistics metri...
Posted by yew1eb <gi...@git.apache.org>.
Github user yew1eb commented on the issue:
https://github.com/apache/flink/pull/4826
[LatencyStatsJob.java](https://gist.github.com/yew1eb/3329239f866b691364f4d11a7f0a846a)
"Source1" and "Source2" Source send a LatencyMarker per 200ms.
"Map-1-A" and "Map-2-B" Map Opreater random sleep a few milliseconds.
The following figure shows the latency statistics of **Map-2-B**:
<img width="1144" alt="wx20171014-230347 2x" src="https://user-images.githubusercontent.com/4133864/31576890-1c7d672c-b0ca-11e7-8abd-f761e072fccd.png">
The following figure shows the latency statistics of **Sink: Print**:
<img width="1158" alt="latency_stat_sink" src="https://user-images.githubusercontent.com/4133864/31576900-67086e72-b0ca-11e7-8247-1c8310e58cf0.png">
R: @zentol @rmetzger
---
[GitHub] flink pull request #4826: [FLINK-7608][metric] Refactoring latency statistic...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4826#discussion_r145114978
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/util/latency/LatencyHistogramStatistics.java ---
@@ -0,0 +1,71 @@
+/*
+ * 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.flink.streaming.util.latency;
+
+import org.apache.flink.metrics.HistogramStatistics;
+
+import org.apache.commons.math3.stat.descriptive.DescriptiveStatistics;
+
+
+/**
+ * Latency histogram statistics implementation returned by {@link LatencyHistogram}.
+ * The statistics class wraps a {@link DescriptiveStatistics} instance and forwards the method calls accordingly.
+ */
+public class LatencyHistogramStatistics extends HistogramStatistics {
+ private final DescriptiveStatistics latencyHistogram;
+
+ public LatencyHistogramStatistics(DescriptiveStatistics latencyHistogram) {
+ this.latencyHistogram = latencyHistogram;
+ }
+
+ @Override
+ public double getQuantile(double quantile) {
+ return latencyHistogram.getPercentile(quantile);
+ }
+
+ @Override
+ public long[] getValues() {
+ // Due to latencyHistogram.getValues() return double[]
+ throw new UnsupportedOperationException();
--- End diff --
probably better to return an empty array instead.
---
[GitHub] flink pull request #4826: [FLINK-7608][metric] Refactoring latency statistic...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4826#discussion_r145376504
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/util/latency/LatencyHistogram.java ---
@@ -0,0 +1,51 @@
+/*
+ * 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.flink.streaming.util.latency;
+
+import org.apache.flink.metrics.Histogram;
+import org.apache.flink.metrics.HistogramStatistics;
+
+import org.apache.commons.math3.stat.descriptive.DescriptiveStatistics;
+
+/**
+ * The {@link LatencyHistogram} use a DescriptiveStatistics {@link DescriptiveStatistics} as a Flink {@link Histogram}.
+ */
+public class LatencyHistogram implements org.apache.flink.metrics.Histogram {
--- End diff --
Only the Histogram and HistogramStatistics should be moved to flink-runtime. They both do not require the LatencyMarker. The `LatencyStats` class remains in flink-streaming-java.
---
[GitHub] flink pull request #4826: [FLINK-7608][metric] Refactoring latency statistic...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4826#discussion_r145114119
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/util/latency/LatencyHistogramStatistics.java ---
@@ -0,0 +1,71 @@
+/*
+ * 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.flink.streaming.util.latency;
+
+import org.apache.flink.metrics.HistogramStatistics;
+
+import org.apache.commons.math3.stat.descriptive.DescriptiveStatistics;
+
+
+/**
+ * Latency histogram statistics implementation returned by {@link LatencyHistogram}.
+ * The statistics class wraps a {@link DescriptiveStatistics} instance and forwards the method calls accordingly.
+ */
+public class LatencyHistogramStatistics extends HistogramStatistics {
--- End diff --
same principle as the latencyHistogram.
---
[GitHub] flink pull request #4826: [FLINK-7608][metric] Refactoring latency statistic...
Posted by yew1eb <gi...@git.apache.org>.
Github user yew1eb commented on a diff in the pull request:
https://github.com/apache/flink/pull/4826#discussion_r145294253
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/util/latency/LatencyHistogramStatistics.java ---
@@ -0,0 +1,71 @@
+/*
+ * 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.flink.streaming.util.latency;
+
+import org.apache.flink.metrics.HistogramStatistics;
+
+import org.apache.commons.math3.stat.descriptive.DescriptiveStatistics;
+
+
+/**
+ * Latency histogram statistics implementation returned by {@link LatencyHistogram}.
+ * The statistics class wraps a {@link DescriptiveStatistics} instance and forwards the method calls accordingly.
+ */
+public class LatencyHistogramStatistics extends HistogramStatistics {
+ private final DescriptiveStatistics latencyHistogram;
+
+ public LatencyHistogramStatistics(DescriptiveStatistics latencyHistogram) {
+ this.latencyHistogram = latencyHistogram;
+ }
+
+ @Override
+ public double getQuantile(double quantile) {
+ return latencyHistogram.getPercentile(quantile);
+ }
+
+ @Override
+ public long[] getValues() {
+ // Due to latencyHistogram.getValues() return double[]
+ throw new UnsupportedOperationException();
--- End diff --
yes, will change.
---
[GitHub] flink issue #4826: [FLINK-7608][metric] Refactoring latency statistics metri...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4826
Looks very good now. Will have to try it out, but i can't spot anything atm 👍
---
[GitHub] flink pull request #4826: [FLINK-7608][metric] Refactoring latency statistic...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4826#discussion_r145113405
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/util/latency/LatencyHistogram.java ---
@@ -0,0 +1,51 @@
+/*
+ * 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.flink.streaming.util.latency;
+
+import org.apache.flink.metrics.Histogram;
+import org.apache.flink.metrics.HistogramStatistics;
+
+import org.apache.commons.math3.stat.descriptive.DescriptiveStatistics;
+
+/**
+ * The {@link LatencyHistogram} use a DescriptiveStatistics {@link DescriptiveStatistics} as a Flink {@link Histogram}.
+ */
+public class LatencyHistogram implements org.apache.flink.metrics.Histogram {
+
+ private final DescriptiveStatistics latencyHistogram;
+
+ public LatencyHistogram(int windowSize) {
+ // 512 element window (4 kb)
--- End diff --
isn't this dependent on the `windowSize` argument?
---
[GitHub] flink pull request #4826: [FLINK-7608][metric] Refactoring latency statistic...
Posted by yew1eb <gi...@git.apache.org>.
Github user yew1eb commented on a diff in the pull request:
https://github.com/apache/flink/pull/4826#discussion_r145370731
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/util/latency/LatencyHistogram.java ---
@@ -0,0 +1,51 @@
+/*
+ * 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.flink.streaming.util.latency;
+
+import org.apache.flink.metrics.Histogram;
+import org.apache.flink.metrics.HistogramStatistics;
+
+import org.apache.commons.math3.stat.descriptive.DescriptiveStatistics;
+
+/**
+ * The {@link LatencyHistogram} use a DescriptiveStatistics {@link DescriptiveStatistics} as a Flink {@link Histogram}.
+ */
+public class LatencyHistogram implements org.apache.flink.metrics.Histogram {
--- End diff --
make sense.
But, the `org.apache.flink.streaming.runtime.streamrecord.LatencyMarker` in flink-streaming-java module, if `DescriptiveStatisticsHistogram` moved to flink-runtime, need add dependency flink-streaming-java for flink-runtime pom.xml.
---
[GitHub] flink issue #4826: [FLINK-7608][metric] Refactor latency statistics metric
Posted by yew1eb <gi...@git.apache.org>.
Github user yew1eb commented on the issue:
https://github.com/apache/flink/pull/4826
ping @zentol
---
[GitHub] flink pull request #4826: [FLINK-7608][metric] Refactoring latency statistic...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4826#discussion_r145113966
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/util/latency/LatencyHistogram.java ---
@@ -0,0 +1,51 @@
+/*
+ * 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.flink.streaming.util.latency;
+
+import org.apache.flink.metrics.Histogram;
+import org.apache.flink.metrics.HistogramStatistics;
+
+import org.apache.commons.math3.stat.descriptive.DescriptiveStatistics;
+
+/**
+ * The {@link LatencyHistogram} use a DescriptiveStatistics {@link DescriptiveStatistics} as a Flink {@link Histogram}.
+ */
+public class LatencyHistogram implements org.apache.flink.metrics.Histogram {
--- End diff --
This class has actually little to do with latency. It's just a wrapper for a DescriptiveStatistics that we happen to use for measuring latency, as such i would rename the class/fields, and move it out of the latency namespace.
---
[GitHub] flink pull request #4826: [FLINK-7608][metric] Refactoring latency statistic...
Posted by yew1eb <gi...@git.apache.org>.
Github user yew1eb commented on a diff in the pull request:
https://github.com/apache/flink/pull/4826#discussion_r145293354
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/util/latency/LatencyHistogramStatistics.java ---
@@ -0,0 +1,71 @@
+/*
+ * 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.flink.streaming.util.latency;
+
+import org.apache.flink.metrics.HistogramStatistics;
+
+import org.apache.commons.math3.stat.descriptive.DescriptiveStatistics;
+
+
+/**
+ * Latency histogram statistics implementation returned by {@link LatencyHistogram}.
+ * The statistics class wraps a {@link DescriptiveStatistics} instance and forwards the method calls accordingly.
+ */
+public class LatencyHistogramStatistics extends HistogramStatistics {
+ private final DescriptiveStatistics latencyHistogram;
+
+ public LatencyHistogramStatistics(DescriptiveStatistics latencyHistogram) {
+ this.latencyHistogram = latencyHistogram;
+ }
+
+ @Override
+ public double getQuantile(double quantile) {
+ return latencyHistogram.getPercentile(quantile);
--- End diff --
sorry, I will fix.
---
[GitHub] flink pull request #4826: [FLINK-7608][metric] Refactoring latency statistic...
Posted by yew1eb <gi...@git.apache.org>.
Github user yew1eb commented on a diff in the pull request:
https://github.com/apache/flink/pull/4826#discussion_r145295629
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/util/latency/LatencyHistogram.java ---
@@ -0,0 +1,51 @@
+/*
+ * 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.flink.streaming.util.latency;
+
+import org.apache.flink.metrics.Histogram;
+import org.apache.flink.metrics.HistogramStatistics;
+
+import org.apache.commons.math3.stat.descriptive.DescriptiveStatistics;
+
+/**
+ * The {@link LatencyHistogram} use a DescriptiveStatistics {@link DescriptiveStatistics} as a Flink {@link Histogram}.
+ */
+public class LatencyHistogram implements org.apache.flink.metrics.Histogram {
--- End diff --
I do not know how to change, could you give some suggestions?
---
[GitHub] flink pull request #4826: [FLINK-7608][metric] Refactoring latency statistic...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4826#discussion_r145115654
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/util/latency/LatencyHistogramStatistics.java ---
@@ -0,0 +1,71 @@
+/*
+ * 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.flink.streaming.util.latency;
+
+import org.apache.flink.metrics.HistogramStatistics;
+
+import org.apache.commons.math3.stat.descriptive.DescriptiveStatistics;
+
+
+/**
+ * Latency histogram statistics implementation returned by {@link LatencyHistogram}.
+ * The statistics class wraps a {@link DescriptiveStatistics} instance and forwards the method calls accordingly.
+ */
+public class LatencyHistogramStatistics extends HistogramStatistics {
+ private final DescriptiveStatistics latencyHistogram;
+
+ public LatencyHistogramStatistics(DescriptiveStatistics latencyHistogram) {
+ this.latencyHistogram = latencyHistogram;
+ }
+
+ @Override
+ public double getQuantile(double quantile) {
+ return latencyHistogram.getPercentile(quantile);
--- End diff --
This is wrong. The quantile must be multiplied by 100, since the range of acceptable values for Histogram#getQuantile is 0-1, whereas for DescriptiveStatistics it is 1-100.
---
[GitHub] flink issue #4826: [FLINK-7608][metric] Refactoring latency statistics metri...
Posted by yew1eb <gi...@git.apache.org>.
Github user yew1eb commented on the issue:
https://github.com/apache/flink/pull/4826
@zentol , Thanks a lot for your review. I had updated the PR according to your comments.
---
[GitHub] flink issue #4826: [FLINK-7608][metric] Refactor latency statistics metric
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4826
So here's the thing: The port of the metric itself is good, and exactly what i want 👍 . What I'm unsatisfied with is the naming of the metric, but that isn't the fault of this PR but a limitation in the metric system.
The way the metric is named is inconsistent. The source task is encoded in the metric name with it's ID, but the target task is encoded in the scope as either ID or name, based on the scope format. (maybe reverse the order but you get the idea i hope).
The conclusion i arrived at is that this metric should not be registered at the task level (which just doesn't support the notion of a metric describing 2 tasks) but at the job level instead (where we can do whatever we want in regards to tasks).
This would, in principal, allow us to have this identifier:
```myjob.latency.source.ABCDE.target.DEFGH.latency_p95```
But this still isn't satisfactory, because it is still hardly usable for key-value reporters, where we want to filter latencies based on the source or target, for which we need something like this:
```
logical scope: job.task.latency.latency_p95:
tags:
job_name = myjob
source = ABCDE
target = DEFGH
```
For this to work however we first have to implement FLINK-7692 to support custom key-value pairs.
As such I would like to delay merging this PR by a week or 2 until the aforementioned feature is implemented, so we don't iterate through 3 different versions.
---
[GitHub] flink pull request #4826: [FLINK-7608][metric] Refactoring latency statistic...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4826#discussion_r145366433
--- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/util/latency/LatencyHistogram.java ---
@@ -0,0 +1,51 @@
+/*
+ * 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.flink.streaming.util.latency;
+
+import org.apache.flink.metrics.Histogram;
+import org.apache.flink.metrics.HistogramStatistics;
+
+import org.apache.commons.math3.stat.descriptive.DescriptiveStatistics;
+
+/**
+ * The {@link LatencyHistogram} use a DescriptiveStatistics {@link DescriptiveStatistics} as a Flink {@link Histogram}.
+ */
+public class LatencyHistogram implements org.apache.flink.metrics.Histogram {
--- End diff --
For the name i would suggest `DescriptiveStatisticsHistogram`. As for the location, `org.apache.flink.runtime.metrics` in flink-runtime, the same place where the `MeterView` class is located.
---