[GitHub] [beam] ajamato commented on a change in pull request #14805: [BEAM-11994] Add HarnessMonitoringInfosRequest/Response InstructionRequest and MonitoringInfosMetadataRequest/Response support to Java SDK.

[GitBox <gi...@apache.org>]

ajamato commented on a change in pull request #14805:
URL: https://github.com/apache/beam/pull/14805#discussion_r655713620



##########
File path: runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/MetricsContainerImpl.java
##########
@@ -84,9 +86,22 @@
   private MetricsMap<KV<MetricName, HistogramData.BucketType>, HistogramCell> histograms =
       new MetricsMap<>(HistogramCell::new);
 
-  /** 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.
+   */
   public MetricsContainerImpl(@Nullable String stepName) {
     this.stepName = stepName;
+    this.isProcessWide = false;

Review comment:
       (no change done)
   Do you want me to add another private ctor?
   Currently there is no ctor defined to call "this(stepName, false)"

##########
File path: runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/MetricsContainerImpl.java
##########
@@ -84,9 +86,22 @@
   private MetricsMap<KV<MetricName, HistogramData.BucketType>, HistogramCell> histograms =
       new MetricsMap<>(HistogramCell::new);
 
-  /** 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.

Review comment:
       No. its not equivalent. We have these in a some other places I believe.
   
   
   It was used here as well
   https://github.com/apache/beam/blob/8463a054c1d7e2b7ee8d11e9569e065cb5e02196/runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/MetricsContainerStepMap.java#L52
   
   I believe this was added for some metrics which had the step already in the label information already. (In some code which was tracking metrics for multiple steps). So there was no need to have the container know about the step name.
   
   Likely one of the system metrics. ElementCount, ExecutionTime, etc.
   
   If you feel this needs to be changed, please pick a specific option and I will introduce it. One I can think of is:
   adding static factory method (which will set the step to null and isProcessWide to true):
   public static MetricsContainerImpl createProcessWideContainer();

##########
File path: runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/MetricsContainerImpl.java
##########
@@ -108,6 +126,11 @@ private void reset(MetricsMap<?, ? extends MetricCell<?>> cells) {
     }
   }
 
+  @Override
+  public boolean isProcessWide() {

Review comment:
       Done. Made it private
   
   (Yes. Previously there was a PR which had called reset() on it which caused some debugging issued when adding this cl.)
   
   

##########
File path: 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());

Review comment:
       The main reason was to allow clearing/cleaning the state between tests.
   
   Explicitly creating it forces you to create a clean state on startup.
   We could have had one here by default as well. I don't feel too strongly here.
   Though I would consider it out of scope of this PR
   




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