You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/06/23 08:58:41 UTC
[GitHub] [beam] davidak09 opened a new pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
davidak09 opened a new pull request #12063:
URL: https://github.com/apache/beam/pull/12063
Beam metrics in Spark history server are not readable because now they're rendered as protobuffer formatted as JSON where metric's value is stored in `bytes` field.
Using already existing `SparkMetricsContainerStepMap` which overrides `toString()` method by calling `SparkBeamMetric.renderAll()` method.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Dataflow | Flink | Samza | Spark
--- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/)
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/)
XLang | --- | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/)
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev removed a comment on pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
aromanenko-dev removed a comment on pull request #12063:
URL: https://github.com/apache/beam/pull/12063#issuecomment-661743260
Run Spark Runner Nexmark Tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev commented on a change in pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on a change in pull request #12063:
URL: https://github.com/apache/beam/pull/12063#discussion_r460134431
##########
File path: runners/spark/src/main/java/org/apache/beam/runners/spark/metrics/SparkMetricsContainerStepMap.java
##########
@@ -27,7 +27,7 @@
@Override
public String toString() {
- return new SparkBeamMetric().renderAll().toString();
+ return asAttemptedOnlyMetricResults(this).toString();
Review comment:
@ibzib Thanks for more details. I see the picture bit still not convinced why `MetricsContainerStepMap::toString ` is used for REST API (if I get it right) - dedicated way to make the protobuf-formatted metrics accessible, as you said, should be better imho.
Are you ok to merge this fix for Spark Runner metrics?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev commented on pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on pull request #12063:
URL: https://github.com/apache/beam/pull/12063#issuecomment-664983381
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev commented on a change in pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on a change in pull request #12063:
URL: https://github.com/apache/beam/pull/12063#discussion_r449044147
##########
File path: runners/spark/src/main/java/org/apache/beam/runners/spark/metrics/MetricsAccumulator.java
##########
@@ -58,13 +57,13 @@ public static void init(SparkPipelineOptions opts, JavaSparkContext jsc) {
opts.isStreaming()
? Optional.of(new CheckpointDir(opts.getCheckpointDir()))
: Optional.absent();
- MetricsContainerStepMap metricsContainerStepMap = new MetricsContainerStepMap();
+ SparkMetricsContainerStepMap metricsContainerStepMap = new SparkMetricsContainerStepMap();
Review comment:
Won't it be enough to just create new instance of `SparkMetricsContainerStepMap` here but keep the same interface of `MetricsContainerStepMap` everywhere as it was before get broken? Why do we need to change it everywhere?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on a change in pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #12063:
URL: https://github.com/apache/beam/pull/12063#discussion_r457644766
##########
File path: runners/spark/src/main/java/org/apache/beam/runners/spark/metrics/SparkMetricsContainerStepMap.java
##########
@@ -27,7 +27,7 @@
@Override
public String toString() {
- return new SparkBeamMetric().renderAll().toString();
+ return asAttemptedOnlyMetricResults(this).toString();
Review comment:
Thanks for the heads up. My assumption in #11369 was that the result was meant to be consumed by code, not humans. After this change, metrics would remain not too readable for Flink. Ideally we could find a way to make the Flink metrics protos available to the Python client without `toString`, since `toString` should generally should be human-readable. The problem is that without a job server, there's nowhere to access job information like metrics outside of what Flink provides natively.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev commented on pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on pull request #12063:
URL: https://github.com/apache/beam/pull/12063#issuecomment-661079575
Run Spark ValidatesRunner
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev commented on pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on pull request #12063:
URL: https://github.com/apache/beam/pull/12063#issuecomment-661743260
Run Spark Runner Nexmark Tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev commented on a change in pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on a change in pull request #12063:
URL: https://github.com/apache/beam/pull/12063#discussion_r449044147
##########
File path: runners/spark/src/main/java/org/apache/beam/runners/spark/metrics/MetricsAccumulator.java
##########
@@ -58,13 +57,13 @@ public static void init(SparkPipelineOptions opts, JavaSparkContext jsc) {
opts.isStreaming()
? Optional.of(new CheckpointDir(opts.getCheckpointDir()))
: Optional.absent();
- MetricsContainerStepMap metricsContainerStepMap = new MetricsContainerStepMap();
+ SparkMetricsContainerStepMap metricsContainerStepMap = new SparkMetricsContainerStepMap();
Review comment:
Won't it be enough to just create new instance of `SparkMetricsContainerStepMap` here but keep the same interface of `MetricsContainerStepMap` everywhere as it was before get broken? Why do we need to change it everywhere since `SparkMetricsContainerStepMap ` just overrides `toString()`?
Also, it would be great to add a test for this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev commented on a change in pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on a change in pull request #12063:
URL: https://github.com/apache/beam/pull/12063#discussion_r460134431
##########
File path: runners/spark/src/main/java/org/apache/beam/runners/spark/metrics/SparkMetricsContainerStepMap.java
##########
@@ -27,7 +27,7 @@
@Override
public String toString() {
- return new SparkBeamMetric().renderAll().toString();
+ return asAttemptedOnlyMetricResults(this).toString();
Review comment:
@ibzib Thanks for more details. I see the picture bit still not convinced why `MetricsContainerStepMap::toString ` is used for REST API (if I get it right) - dedicated way to make the protobuf-formatted metrics accessible, as you said, should be better imho.
Are you ok to merge this fix for Spark metrics?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev commented on pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on pull request #12063:
URL: https://github.com/apache/beam/pull/12063#issuecomment-661079716
Run Spark Runner Nexmark Tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev commented on a change in pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on a change in pull request #12063:
URL: https://github.com/apache/beam/pull/12063#discussion_r457442095
##########
File path: runners/spark/src/main/java/org/apache/beam/runners/spark/metrics/SparkMetricsContainerStepMap.java
##########
@@ -27,7 +27,7 @@
@Override
public String toString() {
- return new SparkBeamMetric().renderAll().toString();
+ return asAttemptedOnlyMetricResults(this).toString();
Review comment:
@ibzib fyi
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev commented on a change in pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on a change in pull request #12063:
URL: https://github.com/apache/beam/pull/12063#discussion_r449044147
##########
File path: runners/spark/src/main/java/org/apache/beam/runners/spark/metrics/MetricsAccumulator.java
##########
@@ -58,13 +57,13 @@ public static void init(SparkPipelineOptions opts, JavaSparkContext jsc) {
opts.isStreaming()
? Optional.of(new CheckpointDir(opts.getCheckpointDir()))
: Optional.absent();
- MetricsContainerStepMap metricsContainerStepMap = new MetricsContainerStepMap();
+ SparkMetricsContainerStepMap metricsContainerStepMap = new SparkMetricsContainerStepMap();
Review comment:
Won't it be enough to just create new instance of `SparkMetricsContainerStepMap` here but keep the same interface of `MetricsContainerStepMap` everywhere as it was before get broken? Why do we need to change it everywhere since `SparkMetricsContainerStepMap ` just overrides `toString()`?
Also, it would be great to add a test for this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev commented on a change in pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on a change in pull request #12063:
URL: https://github.com/apache/beam/pull/12063#discussion_r458073027
##########
File path: runners/spark/src/main/java/org/apache/beam/runners/spark/metrics/SparkMetricsContainerStepMap.java
##########
@@ -27,7 +27,7 @@
@Override
public String toString() {
- return new SparkBeamMetric().renderAll().toString();
+ return asAttemptedOnlyMetricResults(this).toString();
Review comment:
@ibzib Thanks for details. I agree that we should not rely on `toString()` when it's going to be used by other program.
I'm sorry but I'm not sure that I fully understand your last sentence - does it seem that it's only specific Flink-related issue?
Regarding readable Flink metrics - would it be better to fix it on FlinkRunner side, like we do with `SparkMetricsContainerStepMap` for Spark Runner?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on a change in pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #12063:
URL: https://github.com/apache/beam/pull/12063#discussion_r460179800
##########
File path: runners/spark/src/main/java/org/apache/beam/runners/spark/metrics/SparkMetricsContainerStepMap.java
##########
@@ -27,7 +27,7 @@
@Override
public String toString() {
- return new SparkBeamMetric().renderAll().toString();
+ return asAttemptedOnlyMetricResults(this).toString();
Review comment:
> @ibzib Thanks for more details. I see the picture bit still not convinced why MetricsContainerStepMap::toString is used for REST API (if I get it right) - dedicated way to make the protobuf-formatted metrics accessible, as you said, should be better imho.
The reason I used `toString` is because the Flink REST API displays accumulator values using `toString`. I simply wasn't able to come up with a dedicated way to make the protobuf-formatted metrics accessible.
> Are you ok to merge this fix for Spark Runner metrics?
Yes, this is fine for now. I might need to give Flink runner metrics another look to make sure we don't have a similar problem there.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] davidak09 commented on pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
davidak09 commented on pull request #12063:
URL: https://github.com/apache/beam/pull/12063#issuecomment-648008709
R: @dmvk
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev commented on a change in pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on a change in pull request #12063:
URL: https://github.com/apache/beam/pull/12063#discussion_r449044147
##########
File path: runners/spark/src/main/java/org/apache/beam/runners/spark/metrics/MetricsAccumulator.java
##########
@@ -58,13 +57,13 @@ public static void init(SparkPipelineOptions opts, JavaSparkContext jsc) {
opts.isStreaming()
? Optional.of(new CheckpointDir(opts.getCheckpointDir()))
: Optional.absent();
- MetricsContainerStepMap metricsContainerStepMap = new MetricsContainerStepMap();
+ SparkMetricsContainerStepMap metricsContainerStepMap = new SparkMetricsContainerStepMap();
Review comment:
Won't it be enough to just create new instance of `SparkMetricsContainerStepMap` here but keep the same interface of `MetricsContainerStepMap` everywhere as it was before get broken? Why do we need to change it everywhere since `SparkMetricsContainerStepMap ` just overrides `toString()`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev removed a comment on pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
aromanenko-dev removed a comment on pull request #12063:
URL: https://github.com/apache/beam/pull/12063#issuecomment-664983381
Spark Runner Nexmark Tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] davidak09 commented on a change in pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
davidak09 commented on a change in pull request #12063:
URL: https://github.com/apache/beam/pull/12063#discussion_r452823257
##########
File path: runners/spark/src/main/java/org/apache/beam/runners/spark/metrics/SparkMetricsContainerStepMap.java
##########
@@ -27,7 +27,7 @@
@Override
public String toString() {
- return new SparkBeamMetric().renderAll().toString();
+ return asAttemptedOnlyMetricResults(this).toString();
Review comment:
We ran our application built with Beam `2.18.0` and the metrics are displayed correctly, with this fix Spark displays the same results.
The change that broke things up for Spark was part of BEAM-9600 - #11369 .
##########
File path: runners/spark/src/main/java/org/apache/beam/runners/spark/metrics/MetricsAccumulator.java
##########
@@ -58,13 +57,13 @@ public static void init(SparkPipelineOptions opts, JavaSparkContext jsc) {
opts.isStreaming()
? Optional.of(new CheckpointDir(opts.getCheckpointDir()))
: Optional.absent();
- MetricsContainerStepMap metricsContainerStepMap = new MetricsContainerStepMap();
+ SparkMetricsContainerStepMap metricsContainerStepMap = new SparkMetricsContainerStepMap();
Review comment:
You're right, it's enough to change it just in initialization. I fixed it.
I will try to add a regression test.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev merged pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
aromanenko-dev merged pull request #12063:
URL: https://github.com/apache/beam/pull/12063
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev commented on pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on pull request #12063:
URL: https://github.com/apache/beam/pull/12063#issuecomment-663603100
Run Spark Runner Nexmark Tests
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev commented on pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on pull request #12063:
URL: https://github.com/apache/beam/pull/12063#issuecomment-661081237
Run Spark StructuredStreaming ValidatesRunner
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev commented on pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on pull request #12063:
URL: https://github.com/apache/beam/pull/12063#issuecomment-653014984
Run Java PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] davidak09 commented on a change in pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
davidak09 commented on a change in pull request #12063:
URL: https://github.com/apache/beam/pull/12063#discussion_r444071819
##########
File path: runners/spark/src/main/java/org/apache/beam/runners/spark/metrics/MetricsAccumulator.java
##########
@@ -87,13 +86,13 @@ public static MetricsContainerStepMapAccumulator getInstance() {
}
}
- private static Optional<MetricsContainerStepMap> recoverValueFromCheckpoint(
+ private static Optional<SparkMetricsContainerStepMap> recoverValueFromCheckpoint(
JavaSparkContext jsc, CheckpointDir checkpointDir) {
try {
Path beamCheckpointPath = checkpointDir.getBeamCheckpointDir();
checkpointFilePath = new Path(beamCheckpointPath, ACCUMULATOR_CHECKPOINT_FILENAME);
fileSystem = checkpointFilePath.getFileSystem(jsc.hadoopConfiguration());
- MetricsContainerStepMap recoveredValue =
+ SparkMetricsContainerStepMap recoveredValue =
Review comment:
I'm not sure about this - whether it's backward compatible
##########
File path: runners/spark/src/main/java/org/apache/beam/runners/spark/metrics/SparkBeamMetric.java
##########
@@ -51,11 +51,12 @@
}
for (MetricResult<DistributionResult> metricResult : metricQueryResults.getDistributions()) {
DistributionResult result = metricResult.getAttempted();
- metrics.put(renderName(metricResult) + ".count", result.getCount());
- metrics.put(renderName(metricResult) + ".sum", result.getSum());
- metrics.put(renderName(metricResult) + ".min", result.getMin());
- metrics.put(renderName(metricResult) + ".max", result.getMax());
- metrics.put(renderName(metricResult) + ".mean", result.getMean());
+ String name = renderName(metricResult);
+ metrics.put(name + ".count", result.getCount());
+ metrics.put(name + ".sum", result.getSum());
+ metrics.put(name + ".min", result.getMin());
+ metrics.put(name + ".max", result.getMax());
+ metrics.put(name + ".mean", result.getMean());
Review comment:
I'd personally prefer single metric, possibly with `.distribution` suffix, which could include all 5 stats (count, sum, min, max, mean), it would definitely be more readable in Spark UI
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev commented on pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on pull request #12063:
URL: https://github.com/apache/beam/pull/12063#issuecomment-653015794
Run Spark ValidatesRunner
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on a change in pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #12063:
URL: https://github.com/apache/beam/pull/12063#discussion_r460179800
##########
File path: runners/spark/src/main/java/org/apache/beam/runners/spark/metrics/SparkMetricsContainerStepMap.java
##########
@@ -27,7 +27,7 @@
@Override
public String toString() {
- return new SparkBeamMetric().renderAll().toString();
+ return asAttemptedOnlyMetricResults(this).toString();
Review comment:
> @ibzib Thanks for more details. I see the picture bit still not convinced why MetricsContainerStepMap::toString is used for REST API (if I get it right) - dedicated way to make the protobuf-formatted metrics accessible, as you said, should be better imho.
The reason I used `toString` is because the Flink REST API displays accumulator values using `toString`. I simply wasn't able to come up with a dedicated way to make the protobuf-formatted metrics accessible.
> Are you ok to merge this fix for Spark Runner metrics?
Yes, this is fine for now. I might need to give Flink runner metrics another look to fix the equivalent problem there.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on a change in pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #12063:
URL: https://github.com/apache/beam/pull/12063#discussion_r458263018
##########
File path: runners/spark/src/main/java/org/apache/beam/runners/spark/metrics/SparkMetricsContainerStepMap.java
##########
@@ -27,7 +27,7 @@
@Override
public String toString() {
- return new SparkBeamMetric().renderAll().toString();
+ return asAttemptedOnlyMetricResults(this).toString();
Review comment:
There are two ways of running Beam Python Flink/Spark. One way involves starting a Java job server, the other (newer) way does not require a Java job server and instead uses only Python. When using a Java job server, it's easy to get metrics, since the job server has access to the Flink/Spark context object. But in Python, we rely on the Flink REST API to get metrics, so the formatting of the results displayed there is important.
The reason metrics formatting isn't a problem for Spark is that spark_uber_jar_job_server.py just doesn't implement get_metrics yet.
The problem is that if we want to make `MetricsContainerStepMap::toString` human-readable, we'll need to make the protobuf-formatted metrics accessible somewhere else.
Perhaps there is a compromise though if we can somehow format metrics so they are readable both by the proto parser and humans.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aromanenko-dev commented on a change in pull request #12063: [BEAM-10294] using SparkMetricsContainerStepMap for readable metrics presentation in Spark history server UI
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on a change in pull request #12063:
URL: https://github.com/apache/beam/pull/12063#discussion_r449044147
##########
File path: runners/spark/src/main/java/org/apache/beam/runners/spark/metrics/MetricsAccumulator.java
##########
@@ -58,13 +57,13 @@ public static void init(SparkPipelineOptions opts, JavaSparkContext jsc) {
opts.isStreaming()
? Optional.of(new CheckpointDir(opts.getCheckpointDir()))
: Optional.absent();
- MetricsContainerStepMap metricsContainerStepMap = new MetricsContainerStepMap();
+ SparkMetricsContainerStepMap metricsContainerStepMap = new SparkMetricsContainerStepMap();
Review comment:
Won't it be enough to just create new instance of `SparkMetricsContainerStepMap` here but keep the same interface of `MetricsContainerStepMap` everywhere as it was before get broken? Why do we need to change it everywhere since `SparkMetricsContainerStepMap ` just overrides `toString()`?
Also, it would be great to add a regression test for this fix.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org