DO NOT SUBMIT SPLIT INTO OTHER PRsAdd HarnessMonitoringInfosRequest/Response InstructionRequest and MonitoringInfosMetadataRequest/Response support to Java #14490

ajamato · 2021-04-09T03:56:48Z

[BEAM-11994] Add HarnessMonitoringInfosRequest/Response InstructionRequest and MonitoringInfosMetadataRequest/Response support to Java SDK.

Adds InstructionRequest handler for HarnessMonitoringInfosRequest/Response (ported from Python SDK)
Adds start times to each MonitoringInfo recorded (ported from Python SDK)
Modified ShortIdMap to lookup based on the MonitoringInfoMetricName. We can use this in a follow up PR to further optomize the Java SDK Harness, to only construct each MonitoringInfo (URN+labels) pair once.
Enables the InstructionRequest in the Environment

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) 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.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang	ULR	Samza	Twister2
Go	---	---	---
Java
Python	---	---	---
XLang	---	---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---			---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

ajamato · 2021-04-09T03:57:00Z

R: @pabloem

pabloem · 2021-04-14T20:25:08Z

sorry about the delay. taking a look..

pabloem · 2021-04-14T20:30:52Z

Run Java PostCommit

pabloem · 2021-04-14T20:32:21Z

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

@@ -36,4 +38,9 @@

  /** Reset this metric. */
  void reset();
+
+  /** Return the cumulative values for any metrics in this container as MonitoringInfos. */


is this docstring right?

Update the comment

pabloem · 2021-04-14T20:33:06Z

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

+
+  /** Return the cumulative values for any metrics in this container as MonitoringInfos. */
+  default @Nullable DateTime getStartTime() {
+    return null;


Does it make sense to not add a default, and make this not-nullable? (i.e. force all implementers to define a non-null start time?)

I didn't want to add this to all the metrics, because I didn't want to add unncessary calls to the system to get clock times.

Let's add it to just this place first, and consider adding more if they are needed for another project.

pabloem · 2021-04-14T20:35:23Z

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

@@ -302,7 +325,7 @@ public void commitUpdates() {
    ImmutableList.Builder<MetricUpdate<UpdateT>> updates = ImmutableList.builder();
    for (Map.Entry<MetricName, CellT> cell : cells.entries()) {
      UpdateT update = checkNotNull(cell.getValue().getCumulative());
-      updates.add(MetricUpdate.create(MetricKey.create(stepName, cell.getKey()), update));
+      updates.add(MetricUpdate.create(MetricKey.create(stepName, cell.getKey()), update, null));


maybe the start time should be the current time? or maybe not?

pabloem · 2021-04-14T20:42:21Z

runners/jet/src/main/java/org/apache/beam/runners/jet/metrics/JetMetricsContainer.java

@@ -98,7 +98,8 @@ public void flush(boolean async) {
      UpdateT value = cell.getValue();
      if (value != null) {
        MetricKey key = MetricKey.create(stepName, cell.getName());
-        MetricUpdates.MetricUpdate<UpdateT> update = MetricUpdates.MetricUpdate.create(key, value);
+        MetricUpdates.MetricUpdate<UpdateT> update =
+            MetricUpdates.MetricUpdate.create(key, value, null);


does it make sense to get the time from the cell?

No, this is actually now the same MetricCell class. so I can't pull a startTime off it.

its actually a CellT extends AbstractMetric

pabloem · 2021-04-14T20:48:09Z

sdks/java/core/src/main/java/org/apache/beam/sdk/metrics/MetricsContainer.java

+    return false;
+  }
+
+  default void setIsProcessWide(boolean procesWide) {}


if setting does not do anything, maybe we should throw an error instead of ignoring the instruction silently?

pabloem · 2021-04-14T20:49:50Z

...ava/harness/src/main/java/org/apache/beam/fn/harness/control/MonitoringInfoShortIdCache.java

+  private HashMap<MonitoringInfoMetricName, String> infoKeyToShortId = new HashMap<>();
+
+  private HashMap<String, MetricsApi.MonitoringInfo> shortIdToInfo = new HashMap<>();
+


I wonder if these should use a Guava cache with automatic invalidation and so on?

This file was deleted. As @robertwb checked in a different implemetnation in the last few days.

...ava/harness/src/main/java/org/apache/beam/fn/harness/control/MonitoringInfoShortIdCache.java

pabloem · 2021-04-14T20:56:44Z

.../harness/src/main/java/org/apache/beam/fn/harness/control/ProcessWideInstructionHandler.java

+ * MonitoringInfos payloads and MonitoringInfo metadata for "process-wide" metrics, which return
+ * metric values calculated over the life of the process.
+ */
+public class ProcessWideInstructionHandler {


maybe rename this to reference MonitoringInfoMetadataInstructionHandler or something like that? Since it has process-wide and non-process-wide MIs

Renamed to HarnessMonitoringInfosInstructionHandler, which is all this does now.

pabloem · 2021-04-14T21:00:24Z

.../harness/src/main/java/org/apache/beam/fn/harness/control/ProcessWideInstructionHandler.java

+        BeamFnApi.MonitoringInfosMetadataResponse.newBuilder();
+    response.putAllMonitoringInfo(
+        MonitoringInfoShortIdCache.getShortIdCache()
+            .getInfos(request.getMonitoringInfos().getMonitoringInfoIdList()));


It seems that the only moment when a MInfo gets added to the ShortIdCache is when we call getShortId on it, right? I don't see getShortId getting called for the monitoringInfoMetadata, only for the harnessMonitoringInfos - so I wonder how do we get the monitoring infos from the other containers in this call?

That wasn't implemented when I sent you this PR. Though robert's change did introduce it.
I have updated the PR to merge with his implementation.

ajamato · 2021-04-16T18:28:50Z

R: @robertwb
Looks like you had added the ShortID code in this PR
4dd45e0

I will take a look at your change, and try to merge things together.

ajamato · 2021-04-26T20:13:52Z

Run Java_Examples_Dataflow PreCommit

pabloem · 2021-04-28T19:13:51Z

Run Java PreCommit

pabloem

LGTM except for the one comment.

pabloem · 2021-04-28T21:59:00Z

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

+    MonitoringInfo.Builder cleaner = MonitoringInfo.newBuilder(info);
+    cleaner.clearPayload();
+    cleaner.clearStartTime();
+    cleaner.clearType();


isn't the type also necessary to identify a monitoring info? What identifies it / what goes into the hash? Is it hash consistent?

robertwb

This PR seems to do three separate things.

(1) Change the way ShortIdMap works,
(2) Add process-wide metrics, and
(3) add the notion of startTime to a subset of metrics.

It may be preferable to split them up.

robertwb · 2021-04-28T19:12:53Z

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

@@ -45,6 +48,8 @@
   */
  public CounterCell(MetricName name) {
    this.name = name;
+    // The start time of when this cell was first instantied by the container.
+    this.startTime = new DateTime(DateTimeZone.UTC);


Is it a concern that this might be slow?

Moving this to #14805

Yes, revised to only make this call once per process in the new PR

robertwb · 2021-04-28T19:13:12Z

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

@@ -94,6 +99,11 @@ public MetricName getName() {
    return name;
  }

+  @Override
+  public @Nullable DateTime getStartTime() {


If we always initialize it, when would it be null?

Moving this to #14805

I don't want to make the system call to get the time for every metric as you suggested due to performance concerns. So I make only the process wide MetricContainer make this call. So it should be called once for the process.

robertwb · 2021-04-28T19:14:52Z

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

@@ -74,6 +74,8 @@

  protected final @Nullable String stepName;

+  private boolean isProcessWide = false;


Let's make this final. We should know at instantiation time whether it's process wide (given that the step name above is final).

Moving this to #14805

Done in the new PR

robertwb · 2021-04-28T19:15:50Z

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

-  /** Create a new {@link MetricsContainerImpl} associated with the given {@code stepName}. */
+  /**
+   * Create a new {@link MetricsContainerImpl} associated with the given {@code stepName}. If
+   * stepName is null, this MetricsContainer is not bound to a step.


Alternatively, require this be non-null, and add a no-arg constructor for the no-step-name (presumably is-process-wide) case?

Moving this to #14805

Done in the new PR

robertwb · 2021-04-28T19:17:35Z

sdks/java/harness/src/main/java/org/apache/beam/fn/harness/FnHarness.java

@@ -262,6 +265,8 @@ public static void main(
                    }
                  });

+      MetricsEnvironment.setProcessWideContainer(new MetricsContainerImpl(null));


Given that we don't hold onto a reference for this, why don't we have MetricsEnvironment create (if possibly lazily) the process-wide container?

Moving this to #14805

Unfortunately I could not make MetricsEnvironment instantiate a MetricsContainerImpl inside itself as this would create a dep cycle between their packages. So keeping this one as is.

robertwb · 2021-04-28T23:05:33Z

I'm wondering about the value of attaching startTime to each metric. It seems to add a lot of complexity and is a bit ill-defined in the Beam model. Are these tied somehow to process-wide metrics? If so, could we just use the start time of the process itself when publishing these rather than adding the overhead of tracking it per-metric?

…quest and MonitoringInfosMetadataRequest/Response support to Java SDK.

loses critial information. So we must use two maps.

ajamato · 2021-05-13T03:47:36Z

i have split this up into two PRs, rather than 3. As the startTime logic is much simpler now, and only done for the process wide MetricContainer. Overall the whole thing is much smaller. PTAL

(1) Change the way ShortIdMap works,
#14804
(2) Add process-wide metrics, and add the notion of startTime to a subset of metrics.
#14805

robertwb · 2021-05-13T19:09:38Z

Thanks. I'm still not following why we need per-metric start times. And without start times, it seems the (payload-less) BiMap should just work which simplifies things even more.

…

On Wed, May 12, 2021 at 8:47 PM Alex Amato ***@***.***> wrote: i have split this up into two PRs, rather than 3. As the startTime logic is much simpler now, and only done for the process wide MetricContainer. Overall the whole thing is much smaller. PTAL (1) Change the way ShortIdMap works, #14804 <#14804> (2) Add process-wide metrics, and add the notion of startTime to a subset of metrics. #14805 <#14805> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14490 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADWVAKMZQS67DO5YM5FCPTTNNDWNANCNFSM42UFLLAQ> .

ajamato force-pushed the bq_metrics_process_wide branch 7 times, most recently from 32bd70a to 2bbc236 Compare April 10, 2021 05:00

pabloem reviewed Apr 14, 2021

View reviewed changes

ajamato force-pushed the bq_metrics_process_wide branch 5 times, most recently from 813774d to e10e165 Compare April 22, 2021 01:52

ajamato requested a review from pabloem April 22, 2021 01:52

ajamato force-pushed the bq_metrics_process_wide branch from e10e165 to 7f18273 Compare April 27, 2021 20:37

pabloem reviewed Apr 28, 2021

View reviewed changes

robertwb reviewed Apr 28, 2021

View reviewed changes

Alex Amato added 4 commits May 12, 2021 17:30

[BEAM-11994] Add HarnessMonitoringInfosRequest/Response InstructionRe…

6f54a72

…quest and MonitoringInfosMetadataRequest/Response support to Java SDK.

Iterate based on PR comments/suggestions

fa8d4c5

Added the BiMap back to ShortIdMap

99f21e9

Removed the BiMap, as I realized storing only the cleaned MonitoringInfo

25aae15

loses critial information. So we must use two maps.

ajamato force-pushed the bq_metrics_process_wide branch from 7f18273 to 25aae15 Compare May 13, 2021 00:40

ajamato closed this May 13, 2021

		private HashMap<MonitoringInfoMetricName, String> infoKeyToShortId = new HashMap<>();

		private HashMap<String, MetricsApi.MonitoringInfo> shortIdToInfo = new HashMap<>();

		@@ -74,6 +74,8 @@

		protected final @Nullable String stepName;

		private boolean isProcessWide = false;

DO NOT SUBMIT SPLIT INTO OTHER PRsAdd HarnessMonitoringInfosRequest/Response InstructionRequest and MonitoringInfosMetadataRequest/Response support to Java #14490

DO NOT SUBMIT SPLIT INTO OTHER PRsAdd HarnessMonitoringInfosRequest/Response InstructionRequest and MonitoringInfosMetadataRequest/Response support to Java #14490

Conversation

ajamato commented Apr 9, 2021 • edited Loading

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

ajamato commented Apr 9, 2021

pabloem commented Apr 14, 2021

pabloem commented Apr 14, 2021

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

ajamato commented Apr 16, 2021

ajamato commented Apr 26, 2021

pabloem commented Apr 28, 2021

pabloem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertwb left a 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

robertwb commented Apr 28, 2021

ajamato commented May 13, 2021

robertwb commented May 13, 2021 via email

ajamato commented Apr 9, 2021 •

edited

Loading