robertwb commented on a change in pull request #14490:
URL: https://github.com/apache/beam/pull/14490#discussion_r622465920
##########
File path:
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() {
Review comment:
If we always initialize it, when would it be null?
##########
File path:
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;
Review comment:
Let's make this final. We should know at instantiation time whether it's
process wide (given that the step name above is final).
##########
File path:
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);
Review comment:
Is it a concern that this might be slow?
##########
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(null));
Review comment:
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?
##########
File path:
runners/core-java/src/main/java/org/apache/beam/runners/core/metrics/MetricsContainerImpl.java
##########
@@ -84,7 +86,10 @@
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:
Alternatively, require this be non-null, and add a no-arg constructor
for the no-step-name (presumably is-process-wide) case?
--
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:
[email protected]
