[BEAM-6361][BEAM-6364] fix user-metric-prefix checking in Flink portable metrics update #7408

ryan-williams · 2019-01-03T22:48:58Z

two small follow-ups to #7183:

BEAM-6361: fix incorrect user-metric-prefix check
BEAM-6364: WARN instead of throw on as-yet-unsupported metrics types

They're each pretty small, and the tests of the latter depend on the former, so I put them both in this PR; let me know if it would be better if I split them out.

R: @ajamato
R: @robertwb (we'd previously discussed the BEAM-6364 portion of this in particular)

Post-Commit Tests Status (on master branch)

Lang	Apex	Dataflow	Flink	Gearpump	Samza	Spark
Go	---	---	---	---	---	---
Java
Python	---			---	---	---

ryan-williams · 2019-01-03T23:09:56Z

runners/flink/src/test/java/org/apache/beam/runners/flink/metrics/FlinkMetricContainerTest.java

+    when(metricGroup.counter("ns1.metric1")).thenReturn(userCounter);
+
+    SimpleCounter elemCounter = new SimpleCounter();
+    when(metricGroup.counter("beam.metric:element_count:v1")).thenReturn(elemCounter);


The current namespace parsing logic above just takes the everything after the first : as the "name", so this name is metric:element_count:v1.

I don't see any reason that having additional :s in the "name" is a problem, but I saw a note about it (only related to user metrics, though) in SimpleMonitoringInfoBuilder, so open to whether there's a better way to think about it here.

I suppose user-defined counters should not be able to change the namespace via colons.

ryan-williams · 2019-01-04T04:47:40Z

python failure seems unrelated, and the same as in 3385 (#7416), 3382 (#7300), 3370 (#7407), and perhaps others

======================================================================
FAIL: test_error_traceback_includes_user_code (apache_beam.runners.portability.portable_runner_test.PortableRunnerTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Python_Commit@2/src/sdks/python/apache_beam/runners/portability/fn_api_runner_test.py", line 473, in test_error_traceback_includes_user_code
    self.assertIn('second', message)
AssertionError: 'second' not found in 'Traceback (most recent call last):\n  File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Python_Commit@2/src/sdks/python/apache_beam/runners/portability/fn_api_runner_test.py", line 466, in test_error_traceback_includes_user_code\n    p | beam.Create([0]) | beam.Map(first)  # pylint: disable=expression-not-assigned\n  File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Python_Commit@2/src/sdks/python/apache_beam/pipeline.py", line 425, in __exit__\n    self.run().wait_until_finish()\n  File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Python_Commit@2/src/sdks/python/apache_beam/runners/portability/portable_runner.py", line 316, in wait_until_finish\n    self._job_id, self._state, self._last_error_message()))\nRuntimeError: Pipeline job-7dd476c6-fc08-41b1-8516-86e218de84b1 failed in state FAILED: start <DataInputOperation receivers=[ConsumerSet[Create/Read/Impulse.out0, coder=WindowedValueCoder[BytesCoder], len(consumers)=1]]>\n'
-------------------- >> begin captured logging << --------------------

ryan-williams · 2019-01-04T05:19:13Z

Run Python PreCommit

boyuanzz · 2019-01-04T21:39:17Z

python failure seems unrelated, and the same as in 3385 (#7416), 3382 (#7300), 3370 (#7407), and perhaps others

======================================================================
FAIL: test_error_traceback_includes_user_code (apache_beam.runners.portability.portable_runner_test.PortableRunnerTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Python_Commit@2/src/sdks/python/apache_beam/runners/portability/fn_api_runner_test.py", line 473, in test_error_traceback_includes_user_code
    self.assertIn('second', message)
AssertionError: 'second' not found in 'Traceback (most recent call last):\n  File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Python_Commit@2/src/sdks/python/apache_beam/runners/portability/fn_api_runner_test.py", line 466, in test_error_traceback_includes_user_code\n    p | beam.Create([0]) | beam.Map(first)  # pylint: disable=expression-not-assigned\n  File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Python_Commit@2/src/sdks/python/apache_beam/pipeline.py", line 425, in __exit__\n    self.run().wait_until_finish()\n  File "/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Python_Commit@2/src/sdks/python/apache_beam/runners/portability/portable_runner.py", line 316, in wait_until_finish\n    self._job_id, self._state, self._last_error_message()))\nRuntimeError: Pipeline job-7dd476c6-fc08-41b1-8516-86e218de84b1 failed in state FAILED: start <DataInputOperation receivers=[ConsumerSet[Create/Read/Impulse.out0, coder=WindowedValueCoder[BytesCoder], len(consumers)=1]]>\n'
-------------------- >> begin captured logging << --------------------

This error is tracking in https://issues.apache.org/jira/browse/BEAM-6280.

mxm

Looks good to me. Just a minor comment.

...ore-java/src/main/java/org/apache/beam/runners/core/metrics/SimpleMonitoringInfoBuilder.java

mxm · 2019-01-23T22:26:46Z

runners/flink/src/test/java/org/apache/beam/runners/flink/metrics/FlinkMetricContainerTest.java

+    when(metricGroup.counter("ns1.metric1")).thenReturn(userCounter);
+
+    SimpleCounter elemCounter = new SimpleCounter();
+    when(metricGroup.counter("beam.metric:element_count:v1")).thenReturn(elemCounter);


I suppose user-defined counters should not be able to change the namespace via colons.

angoenka · 2019-01-24T23:21:06Z

Run Portable_Python PreCommit

ryan-williams · 2019-01-25T00:27:38Z

Run Java PreCommit

ryan-williams · 2019-01-25T00:28:16Z

org.apache.beam.sdk.io.elasticsearch.ElasticsearchIOTest > testWriteRetryValidRequest FAILED
    java.lang.AssertionError at ElasticsearchIOTest.java:219

mxm · 2019-01-25T00:44:28Z

If it's only that test failing, it's fine to merge. All other tests still run.

ryan-williams · 2019-01-25T04:34:51Z

Looks like everything passed the second time around, so this should be good to go

ryan-williams mentioned this pull request Jan 3, 2019

[BEAM-6165] send metrics to Flink in portable Flink runner #7183

Merged

ryan-williams commented Jan 3, 2019

View reviewed changes

ryan-williams changed the title ~~[BEAM-6361] fix user-metric-prefix checking in Flink portable metrics update~~ [BEAM-6361][BEAM-6364] fix user-metric-prefix checking in Flink portable metrics update Jan 4, 2019

ryan-williams force-pushed the flink branch from e9e0311 to f266fec Compare January 4, 2019 03:57

mxm approved these changes Jan 23, 2019

View reviewed changes

fix user-metric-prefix checking in Flink portable metrics update

0808509

ryan-williams force-pushed the flink branch from f266fec to 0a47e1a Compare January 24, 2019 20:34

warn instead of throw in case of unsupported portable flink metrics

5951cca

ryan-williams force-pushed the flink branch from 0a47e1a to 5951cca Compare January 24, 2019 22:54

mxm merged commit c51f107 into apache:master Jan 25, 2019

ryan-williams mentioned this pull request Jan 26, 2019

[BEAM-6165] FlinkMetricContainer cleanup #7637

Merged

kennknowles mentioned this pull request Jun 3, 2022

Flaky python3 test in Python PreCommit #19319

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-6361][BEAM-6364] fix user-metric-prefix checking in Flink portable metrics update #7408

[BEAM-6361][BEAM-6364] fix user-metric-prefix checking in Flink portable metrics update #7408

ryan-williams commented Jan 3, 2019 •

edited

Loading

ryan-williams Jan 3, 2019

mxm Jan 23, 2019

ryan-williams commented Jan 4, 2019 •

edited

Loading

ryan-williams commented Jan 4, 2019

boyuanzz commented Jan 4, 2019

mxm left a comment

mxm Jan 23, 2019

angoenka commented Jan 24, 2019

ryan-williams commented Jan 25, 2019

ryan-williams commented Jan 25, 2019

mxm commented Jan 25, 2019

ryan-williams commented Jan 25, 2019 •

edited

Loading

[BEAM-6361][BEAM-6364] fix user-metric-prefix checking in Flink portable metrics update #7408

[BEAM-6361][BEAM-6364] fix user-metric-prefix checking in Flink portable metrics update #7408

Conversation

ryan-williams commented Jan 3, 2019 • edited Loading

Post-Commit Tests Status (on master branch)

ryan-williams Jan 3, 2019

Choose a reason for hiding this comment

mxm Jan 23, 2019

Choose a reason for hiding this comment

ryan-williams commented Jan 4, 2019 • edited Loading

ryan-williams commented Jan 4, 2019

boyuanzz commented Jan 4, 2019

mxm left a comment

Choose a reason for hiding this comment

mxm Jan 23, 2019

Choose a reason for hiding this comment

angoenka commented Jan 24, 2019

ryan-williams commented Jan 25, 2019

ryan-williams commented Jan 25, 2019

mxm commented Jan 25, 2019

ryan-williams commented Jan 25, 2019 • edited Loading

ryan-williams commented Jan 3, 2019 •

edited

Loading

ryan-williams commented Jan 4, 2019 •

edited

Loading

ryan-williams commented Jan 25, 2019 •

edited

Loading