[GitHub] [beam] ajamato commented on a change in pull request #13429: Add start_times to MonitoringInfos and populate them in the python SDK

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

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



##########
File path: sdks/python/apache_beam/metrics/cells.py
##########
@@ -77,6 +79,13 @@ def reset(self):
     # type: () -> None
     raise NotImplementedError
 
+  @property
+  def start_time(self):

Review comment:
       I tried doing this, but couldn't make it simpler. I was going to make the base class return an empty MonitoringInfo except for setting the start time, then updating it. But since the implementations in the subcalsses are calling into functions which return a whole monitoring_info (rather than setting fields on one passed in), I found what I could produce wasn't actually cleaner.
   
   
   
   i.e. calling 
         mi = monitoring_infos.int64_user_counter(
             name.namespace,
             name.name,
             self.get_cumulative(),
             ptransform=transform_id)
   
   If you had something else in mind, please provide a snippet, and ill see what i can do

##########
File path: model/pipeline/src/main/proto/metrics.proto
##########
@@ -401,6 +402,17 @@ message MonitoringInfo {
   // as Stackdriver will be able to aggregate the metrics using a subset of the
   // provided labels
   map<string, string> labels = 4;
+
+  // This indicates when the first value was populated by the SDK Harness.

Review comment:
       Done




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