[BEAM-6181] Implemented msec counters support in FnApi world. #7323

Ardagan · 2018-12-19T18:58:22Z

Minor refactoring to generalize validation and
counter transformation.

Follow this checklist to help us incorporate your contribution quickly and easily:

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.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

Post-Commit Tests Status (on master branch)

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

Ardagan · 2018-12-19T19:06:26Z

@ajamato

Ardagan · 2018-12-20T17:27:26Z

Run Portable_Python PreCommit

Ardagan · 2018-12-20T17:36:40Z

Run Portable_Python PreCommit

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

ajamato · 2018-12-20T18:23:43Z

...java/src/test/java/org/apache/beam/runners/core/metrics/SpecMonitoringInfoValidatorTest.java

+
+  @Before
+  public void setUp() throws Exception {
+    testObject = new SpecMonitoringInfoValidator();


rename test object to validator please

It is one of approaches to implementing unittests, where you name object you're testing explicitly as testObject. This usually simplifies reading test since you always know what operations are called for tested obect and which are part of setup.

ajamato · 2018-12-20T18:24:09Z

...java/src/test/java/org/apache/beam/runners/core/metrics/SpecMonitoringInfoValidatorTest.java

+  }
+
+  @Test
+  public void validateReturnsErrorOnInvalidMonitoringInfoType() {


Does this test return an error? Seems like it is successful

Just seems like the tests are a bit inverted.

SpecMonitoringInfoValidator has only one method "validate", so I skip it in test names.
Test verifies that "validate" method returns error on invalid MonitoringInfo type received.

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

ajamato · 2018-12-20T18:28:10Z

...java/src/test/java/org/apache/beam/runners/core/metrics/SpecMonitoringInfoValidatorTest.java

+
+    testInput =
+        MonitoringInfo.newBuilder()
+            .setUrn("beam:metric:element_count:v1")


Can you use the constants please
SimpleMonitoringInfoBuilder.ELEMENT_COUNT_URN
SimpleMonitoringInfoBuilder.USER_COUNTER_PREFIX

I was thinking of this, but since these constants will be declared outside of test body, it will make test less readable.
Do you think it is worth doing?

I would like the shared constants to be used as much has possible. So there is a single place where it is defined, incase it needs to be chagned.

ajamato · 2018-12-20T19:10:40Z

...he/beam/runners/dataflow/worker/fn/control/UserMonitoringInfoToCounterUpdateTransformer.java

+    this.specValidator = specMonitoringInfoValidator;
+  }
+
+  static final String BEAM_METRICS_USER_PREFIX =


This is defined in two files now, SimpleMonitoringInfoBuilder. Can we just define it in one place

I see it as this value is defined in one place -- beam_fn_api.proto and both, SimpleMonitoringInfoBuilder and UserMonitoringInfoToCounterUpdateTransformer only define convenient accessor to the field.

I think that creating a structure with these fields separate from proto will only bring more inconvenience to the code.

Then can you use a shared static helper method to access it, so we don't duplicate the 4 getter calls below.
i.e. something like.

static final String BEAM_METRICS_USER_PREFIX = SimpleMonitoringInfo,getUrn(Enum.USER_COUNTER);

Synced offline. BeamUrns extracts value from MonitoringInfoUrns, while UserMonitoringInfoToCounterUpdateTransformer utilizes MonitoringInfoSpecs. Decided to keep this PR as-is and have a separate PR to remove MonitoringInfoUrns enum and utilize MonitoringInfoSpecs as source of truth.

ajamato · 2018-12-20T19:21:11Z

...he/beam/runners/dataflow/worker/fn/control/MSecMonitoringInfoToCounterUpdateTransformer.java

+
+    String urn = monitoringInfo.getUrn();
+    if (!urnToCounterNameMapping.keySet().contains(urn)) {
+      throw new RuntimeException(String.format("Received unexpected counter urn: %s", urn));


Please just drop metrics if they are invalid, don't throw an exception. This would be bad for many of our users, who don't care about metrics but just want their pipeline to finish properly.

This exception doesn't validate MonitoringInfo, instead it checks validity of logic.
If MSecMonitoringInfoToCounterUpdateTransformer receives non-supported counter it is not a problem of counter, but invoking code.

...he/beam/runners/dataflow/worker/fn/control/MSecMonitoringInfoToCounterUpdateTransformer.java

ajamato · 2018-12-20T19:32:27Z

...he/beam/runners/dataflow/worker/fn/control/MSecMonitoringInfoToCounterUpdateTransformer.java

+              + monitoringInfo.toString());
+    }
+
+    return Optional.empty();


Not sure on this style of returning a string error. Maybe another reviewer can comment.

...eam/runners/dataflow/worker/fn/control/MSecMonitoringInfoToCounterUpdateTransformerTest.java

ajamato · 2019-01-10T00:02:10Z

...java/src/test/java/org/apache/beam/runners/core/metrics/SpecMonitoringInfoValidatorTest.java

+
+    testInput =
+        MonitoringInfo.newBuilder()
+            .setUrn("beam:metric:element_count:v1")


I would like the shared constants to be used as much has possible. So there is a single place where it is defined, incase it needs to be chagned.

ajamato · 2019-01-10T00:06:13Z

...he/beam/runners/dataflow/worker/fn/control/UserMonitoringInfoToCounterUpdateTransformer.java

+    this.specValidator = specMonitoringInfoValidator;
+  }
+
+  static final String BEAM_METRICS_USER_PREFIX =


Then can you use a shared static helper method to access it, so we don't duplicate the 4 getter calls below.
i.e. something like.

static final String BEAM_METRICS_USER_PREFIX = SimpleMonitoringInfo,getUrn(Enum.USER_COUNTER);

...e/beam/runners/dataflow/worker/fn/control/FnApiMonitoringInfoToCounterUpdateTransformer.java

ajamato · 2019-01-10T00:18:41Z

...he/beam/runners/dataflow/worker/fn/control/UserMonitoringInfoToCounterUpdateTransformer.java

+
+    String nameWithNamespace = urn.substring(BEAM_METRICS_USER_PREFIX.length()).replace("^:", "");
+
+    final int lastColonIndex = nameWithNamespace.lastIndexOf(':');


I think that I wrote the same parsing code in one my PRs. You can submit yours first, but please add a TODO comment here to use a shared method for parsing the user metric format, and I'll take care of it,

...-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/WorkItemStatusClient.java

.../src/main/java/org/apache/beam/runners/dataflow/worker/fn/control/BeamFnMapTaskExecutor.java

Ardagan · 2019-01-17T18:48:47Z

Rebased on top of master.
+committer: @swegner

swegner · 2019-01-22T20:41:11Z

Run Java PreCommit

Ardagan · 2019-01-22T22:29:01Z

Run Python PreCommit

Ardagan · 2019-01-22T22:43:34Z

Update on failures:

I ran ./gradlew :beam-runners-direct-java:needsRunnerTests locally and it passes. Seem to be flake.

Looking on how can I run python tests. Running :pythonPreCommit fails installing docs deps. Checking how can I repro the error.

Minor refactoring to generalize validation and counter transformation.

Ardagan · 2019-01-22T22:45:11Z

Rebased on master. See if this was some merge issue.

swegner · 2019-01-23T16:04:33Z

The Java failure is a known-issue failing on master. I believe this is ok to merge.

Ardagan force-pushed the MetricsMsec branch from 92b3642 to aff3353 Compare December 19, 2018 19:05

ajamato reviewed Dec 20, 2018

View reviewed changes

ajamato reviewed Jan 10, 2019

View reviewed changes

Ardagan force-pushed the MetricsMsec branch from 42d1290 to 78eff6a Compare January 17, 2019 18:48

ajamato approved these changes Jan 22, 2019

View reviewed changes

Mikhail Gryzykhin added 3 commits January 22, 2019 14:44

Implemented msec counters support in FnApi world.

150f5c7

Minor refactoring to generalize validation and counter transformation.

run checkstyle, spotless, etc

85970a0

Use constants

38c6367

Ardagan force-pushed the MetricsMsec branch from 62439ce to 38c6367 Compare January 22, 2019 22:44

swegner merged commit 8b4898e into apache:master Jan 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-6181] Implemented msec counters support in FnApi world. #7323

[BEAM-6181] Implemented msec counters support in FnApi world. #7323

Ardagan commented Dec 19, 2018

Ardagan commented Dec 19, 2018

Ardagan commented Dec 20, 2018

Ardagan commented Dec 20, 2018

ajamato Dec 20, 2018

Ardagan Dec 26, 2018

ajamato Dec 20, 2018

ajamato Dec 20, 2018

Ardagan Dec 26, 2018

ajamato Dec 20, 2018

Ardagan Dec 26, 2018

ajamato Jan 10, 2019

ajamato Dec 20, 2018

Ardagan Dec 26, 2018

ajamato Jan 10, 2019

Ardagan Jan 17, 2019

ajamato Dec 20, 2018

Ardagan Dec 20, 2018

ajamato Dec 20, 2018

ajamato Jan 10, 2019

ajamato Jan 10, 2019

ajamato Jan 10, 2019

Ardagan Jan 17, 2019

Ardagan commented Jan 17, 2019

swegner commented Jan 22, 2019

Ardagan commented Jan 22, 2019

Ardagan commented Jan 22, 2019

Ardagan commented Jan 22, 2019

swegner commented Jan 23, 2019


		String nameWithNamespace = urn.substring(BEAM_METRICS_USER_PREFIX.length()).replace("^:", "");

		final int lastColonIndex = nameWithNamespace.lastIndexOf(':');

[BEAM-6181] Implemented msec counters support in FnApi world. #7323

[BEAM-6181] Implemented msec counters support in FnApi world. #7323

Conversation

Ardagan commented Dec 19, 2018

Post-Commit Tests Status (on master branch)

Ardagan commented Dec 19, 2018

Ardagan commented Dec 20, 2018

Ardagan commented Dec 20, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ardagan commented Jan 17, 2019

swegner commented Jan 22, 2019

Ardagan commented Jan 22, 2019

Ardagan commented Jan 22, 2019

Ardagan commented Jan 22, 2019

swegner commented Jan 23, 2019