[BEAM-6138] Add User Counter Metric Support to Java SDK by ajamato · Pull Request #6799 · apache/beam

ajamato · 2018-10-23T21:17:22Z

Please add a meaningful description for your change here
Adds Java User Metrics to the Java SDK.

Metrics set using Metrics.counter, inside the bundle processing thread (start, finish, process) will be collected and reported in the ProcesBundleResponse.

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	---			---	---	---

ajamato · 2018-11-28T18:43:48Z

This is ready for review now. There are two commits because this depends on a separate PR to be submitted first (#6786).

PTAL appreciate the review :).
@robertwb @lukecwik @echauchot

ajamato · 2018-11-29T18:08:00Z

@Ardagan

Ardagan

I tried to avoid commenting on changes that are pending in PR 6786.

Ardagan · 2018-11-29T18:47:18Z

sdks/java/harness/src/main/java/org/apache/beam/fn/harness/control/ProcessBundleHandler.java

+        }
+
+        // Extract user metrics and store as MonitoringInfos.
+        MetricUpdates mus = metricsContainer.getUpdates();


extract response building to separate method.

sdks/java/harness/src/main/java/org/apache/beam/fn/harness/control/ProcessBundleHandler.java

Ardagan · 2018-11-30T00:11:57Z

model/fn-execution/src/main/proto/beam_fn_api.proto

+
+// Enum extension to store the MonitoringInfoSpecs.
+extend google.protobuf.EnumValueOptions {
+  MonitoringInfoSpec monitoring_info_spec = 207174266;


Do we have any guidelines on how to generate this number to avoid collisions? I failed to find good docs on this.

I believe that we should use the commit number, and converted it from hex to dec.

Ardagan · 2018-11-30T00:22:01Z

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

+ *
+ * <p>Example Usage (ElementCount counter):
+ *
+ * <p>SimpleMonitoringInfoBuilder builder = new SimpleMonitoringInfoBuilder();


If there is a way to preserve formatting on autoformat, it would be great to keep setters on new lines.
I didn't find a way to properly invoke relevant formatter, so can't suggest a solution here atm.

I think that we would need to reconfigure spotless. @swegner any ideas here?

Our spotless config is here, which currently just used googleJavaFormat(). My preference would be to generally limit our customization and deviate only where it's important. GitHub isn't showing me the original diff so I can't tell exactly what's going wrong here.

There are also various javadoc constructs which could help; this article has some guidance: A Guide to Formatting Code Snippets in Javadoc

Ardagan · 2018-11-30T05:31:35Z

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

+  }
+
+  /**
+   * Builds the provided MonitoringInfo, if validateAndDropInvalid is set then it drops any


Builds the provided MonitoringInfo.
Returns null if validateAndDropInvalid set and fields do not match respecting MonitoringInfoSpec based on urn.

Ardagan · 2018-11-30T05:34:24Z

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

+  }
+
+  /**
+   * @param namespace The namespace of the metric.


[Ignore if adding @param tags is required by linter]
Remove these param specs, since you do not provide any extra information here.
You can rename name and namespace to metricName and metricNamespace respectively, if you think this clarification is required.
Same for other cases.

Ardagan · 2018-11-30T05:58:48Z

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

+public class SimpleMonitoringInfoBuilderTest {
+
+  @Test
+  public void testSpecNotFullyMet() {


Please rename to "testReturnsNullIfSpecRequirementsNotMet"
same for other test methods.

Ardagan · 2018-11-30T06:01:20Z

...execution/src/test/java/org/apache/beam/runners/fnexecution/control/RemoteExecutionTest.java

+            }
+
+            SimpleMonitoringInfoBuilder builder = new SimpleMonitoringInfoBuilder();
+            builder.setUrnForUserMetric(RemoteExecutionTest.class.getName(), "counterMetric");


Extract counter name to constant.

Ardagan · 2018-11-30T06:19:36Z

sdks/java/harness/src/test/java/org/apache/beam/fn/harness/FnApiDoFnRunnerTest.java

+    Iterables.getOnlyElement(startFunctions).run();
+    mainOutputValues.clear();
+
+    assertThat(consumers.keySet(), containsInAnyOrder(inputPCollectionId, outputPCollectionId));


We are testing metrics here, so we can skip other check.

ajamato

Couldn't seem to comment inline about moving the declaration in ProcessBundleHandler
ProcessBundleResponse.Builder response = ProcessBundleResponse.newBuilder();

This can't be moved because it is used outside of the try block at the end, so this is the closest place it can be defined

ajamato · 2018-12-07T21:14:48Z

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

+ *
+ * <p>Example Usage (ElementCount counter):
+ *
+ * <p>SimpleMonitoringInfoBuilder builder = new SimpleMonitoringInfoBuilder();


I think that we would need to reconfigure spotless. @swegner any ideas here?

ajamato · 2018-12-07T21:15:53Z

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

+  }
+
+  /**
+   * @param namespace The namespace of the metric.


ajamato · 2018-12-07T21:16:36Z

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

+  }
+
+  /**
+   * Builds the provided MonitoringInfo, if validateAndDropInvalid is set then it drops any


ajamato · 2018-12-07T21:18:11Z

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

+public class SimpleMonitoringInfoBuilderTest {
+
+  @Test
+  public void testSpecNotFullyMet() {


ajamato · 2018-12-07T21:21:51Z

...execution/src/test/java/org/apache/beam/runners/fnexecution/control/RemoteExecutionTest.java

+            }
+
+            SimpleMonitoringInfoBuilder builder = new SimpleMonitoringInfoBuilder();
+            builder.setUrnForUserMetric(RemoteExecutionTest.class.getName(), "counterMetric");


ajamato · 2018-12-07T21:29:47Z

sdks/java/harness/src/test/java/org/apache/beam/fn/harness/FnApiDoFnRunnerTest.java

+    Iterables.getOnlyElement(startFunctions).run();
+    mainOutputValues.clear();
+
+    assertThat(consumers.keySet(), containsInAnyOrder(inputPCollectionId, outputPCollectionId));


ajamato · 2018-12-07T21:42:40Z

@swegner Would you like to review as well?

ajamato · 2018-12-07T21:44:11Z

Retest this please

Ardagan · 2018-12-07T22:36:35Z

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

+
+  static {
+    for (MonitoringInfoSpecs.Enum val : MonitoringInfoSpecs.Enum.values()) {
+      // Ignore the UNRECOGNIZED = -1 value;


This comment is redundant. It is better to remove it, or replace with explanation why we ignore "UNRECOGNIZED" values.

Ardagan · 2018-12-07T22:40:25Z

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

+      return false;
+    }
+
+    Set<String> requiredLabels = new HashSet<String>(spec.getRequiredLabelsList());


We can also add validation of different labels values. Such as PTransform should be a valid pTransform name, etc.

I'll put a TODO in that, to enhance it. Let's consider that out of scope for now.

Ardagan · 2018-12-07T22:48:04Z

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

+   *
+   * @param pTransform
+   */
+  public void setPTransformLabel(String pTransform) {


It is better to define generalized method setLabel(String labelName, String labelValue) and provide a list of standard label names.
This is one of examples that might work
https://stackoverflow.com/questions/3978654/best-way-to-create-enum-of-strings

Done, I think this is more or less what you mean

Ardagan · 2018-12-07T22:49:53Z

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

+    assertNull(builder.build());
+
+    builder.setPCollectionLabel("myPcollection");
+    assertTrue(builder.build() != null);


Would be good idea to validate result values here as well.

Ardagan · 2018-12-07T22:54:00Z

sdks/java/harness/src/main/java/org/apache/beam/fn/harness/control/ProcessBundleHandler.java

        new UnknownPTransformRunnerFactory(urnToPTransformRunnerFactoryMap.keySet());
  }

+  private void extractMonitoringInfosToResponse(


This method should build a list of MonitoringInfos and caller should set MonitoringInfos to response.
Doing this will simplify reading code greatly and prevent further complexity growth.

I'm using this style to prevent putting it on a temp list. We could return an iterator object instead?

Iterator would be good.

Returning an iterable now, moved it to MetricContainerImpl

This is subset of code from apache#6799 Extracting it for unblocking counter validation changes in Dataflow Java Runner.

ajamato · 2018-12-11T02:13:33Z

@robertwb @echauchot @Ardagan

ajamato · 2018-12-11T17:39:14Z

Retest this please

Ardagan

LGTM. with one comment.

Ardagan · 2018-12-11T19:06:14Z

sdks/java/harness/src/main/java/org/apache/beam/fn/harness/control/ProcessBundleHandler.java

        new UnknownPTransformRunnerFactory(urnToPTransformRunnerFactoryMap.keySet());
  }

+  private void extractMonitoringInfosToResponse(


Iterator would be good.

echauchot

Nice to see that there is portable user metrics now (at least counter) !
I only took a quick overlook. The only question I have is: If I understand correctly metrics updates are only sent at the end of the bundle processing. Is it planed to add metrics updates as part of the status updates during the bundle processing?

…n an iterable Spotless

ajamato · 2018-12-12T21:20:02Z

Yes @echauchot later on we will enhance this to also send metrics on the ProcessBundleProgressResponse, instead of just the ProcessBundleResponse.

swegner · 2018-12-13T18:10:01Z

LGTM. I am going to merge this. but @echauchot if you have additional questions feel free to keep the conversation going.

ajamato force-pushed the java_sdk_metrics branch from b0b40eb to 3c7da5e Compare October 23, 2018 22:27

ajamato force-pushed the java_sdk_metrics branch from 63e2de1 to c9d1dec Compare November 19, 2018 19:15

ajamato force-pushed the java_sdk_metrics branch 10 times, most recently from 50050a1 to 4806ac9 Compare November 27, 2018 22:12

ajamato changed the title ~~Do not submit (yet) Java SDK User Metrics~~ [BEAM-6138] Add User Metric Supoprt to Java SDK (DO NOT SUBMIT YET) Nov 27, 2018

ajamato force-pushed the java_sdk_metrics branch 7 times, most recently from ee0eb8a to de6304a Compare November 28, 2018 18:37

ajamato changed the title ~~[BEAM-6138] Add User Metric Supoprt to Java SDK (DO NOT SUBMIT YET)~~ [BEAM-6138] Add User Metric Supoprt to Java SDK Nov 28, 2018

ajamato force-pushed the java_sdk_metrics branch 2 times, most recently from 4f9fcde to c5ef127 Compare November 28, 2018 19:00

ajamato changed the title ~~[BEAM-6138] Add User Metric Supoprt to Java SDK~~ [BEAM-6138] Add User Metric Support to Java SDK Nov 29, 2018

ajamato changed the title ~~[BEAM-6138] Add User Metric Support to Java SDK~~ [BEAM-6138] Add User Counter Metric Support to Java SDK Nov 29, 2018

Ardagan reviewed Nov 30, 2018

View reviewed changes

ryan-williams mentioned this pull request Dec 3, 2018

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

Merged

Ardagan mentioned this pull request Dec 4, 2018

[BEAM-6181] Reporting user counters via MonitoringInfos in Portable Dataflow Runner. #7202

Merged

2 tasks

ajamato force-pushed the java_sdk_metrics branch from c5ef127 to 891a795 Compare December 7, 2018 19:36

ajamato commented Dec 7, 2018

View reviewed changes

ajamato force-pushed the java_sdk_metrics branch from 1779944 to bf60e00 Compare December 7, 2018 21:41

Ardagan requested changes Dec 7, 2018

View reviewed changes

Ardagan pushed a commit to Ardagan/beam that referenced this pull request Dec 7, 2018

Add monitoring info specs.

5ad2af9

This is subset of code from apache#6799 Extracting it for unblocking counter validation changes in Dataflow Java Runner.

Ardagan mentioned this pull request Dec 7, 2018

Add monitoring info specs. #7232

Closed

2 tasks

ajamato force-pushed the java_sdk_metrics branch from bf60e00 to c314dfe Compare December 11, 2018 01:58

ajamato force-pushed the java_sdk_metrics branch 2 times, most recently from a0cf246 to 50dbf6d Compare December 11, 2018 18:41

Ardagan approved these changes Dec 11, 2018

View reviewed changes

echauchot reviewed Dec 12, 2018

View reviewed changes

Alex Amato added 3 commits December 12, 2018 11:20

[BEAM-6138] Add User Counter Metric Support to Java SDK

f7dd518

spotless

13d1206

Move MonitoringInfo extraction to the MetricsContainerImpl, and retur…

f7ab80a

…n an iterable Spotless

ajamato force-pushed the java_sdk_metrics branch from 4fc030c to f7ab80a Compare December 12, 2018 19:22

fix style

44786a7

swegner self-requested a review December 13, 2018 18:08

swegner approved these changes Dec 13, 2018

View reviewed changes

swegner merged commit 0b3b9e0 into apache:master Dec 13, 2018

Conversation

ajamato commented Oct 23, 2018

Metrics set using Metrics.counter, inside the bundle processing thread (start, finish, process) will be collected and reported in the ProcesBundleResponse.

Post-Commit Tests Status (on master branch)

Uh oh!

ajamato commented Nov 28, 2018

Uh oh!

ajamato commented Nov 29, 2018

Uh oh!

Ardagan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajamato left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajamato commented Dec 7, 2018

Uh oh!

ajamato commented Dec 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!