[GitHub] [beam] ajamato commented on a change in pull request #12883: [BEAM-7746] Add more typing to metrics

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

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



##########
File path: sdks/python/apache_beam/metrics/cells.py
##########
@@ -29,8 +29,12 @@
 import threading
 import time
 from builtins import object
+from typing import TYPE_CHECKING
 from typing import Optional
 
+if TYPE_CHECKING:
+  from apache_beam.metrics.metricbase import MetricName

Review comment:
       I don't really undersand the reason for the conditional import. Whether or not TYPE_CHECKING is enabled, this file still depends on MetricName

##########
File path: sdks/python/apache_beam/metrics/metric.py
##########
@@ -101,23 +121,26 @@ def gauge(namespace, name):
   class DelegatingCounter(Counter):
     """Metrics Counter that Delegates functionality to MetricsEnvironment."""
     def __init__(self, metric_name):
+      # type: (MetricName) -> None
       super(Metrics.DelegatingCounter, self).__init__()
       self.metric_name = metric_name
-      self.inc = MetricUpdater(cells.CounterCell, metric_name, default=1)
+      self.inc = MetricUpdater(cells.CounterCell, metric_name, default=1)  # type: ignore[assignment]

Review comment:
       Is it not possible to override the behaviour by assigning self.inc to something else in the subclass?
   
   Not saying I am a fan of this pattern or anything. But I think its still possible to override it that way
   
   (Though I don't think it should be addressed in this PR)

##########
File path: sdks/python/apache_beam/metrics/metricbase.py
##########
@@ -78,7 +80,7 @@ def __hash__(self):
 
 class Metric(object):
   """Base interface of a metric object."""
-  pass
+  metric_name = None  # type: MetricName

Review comment:
       Its not due to multiple inheritance, its just self.metric_name is simply not set in the base Metric class, but only in the DelegatingCounter, DelegatingDistribution and DelegatingGauge class. (Which have an __init__ which takes a metric_name and assigns it to self). The rest of the code in the repo is assuming self.metric_name is set, as they only deal with the subclasses.
   
   I recommend adding __init__ with a metric_name parameter to Metric and its subclasses (Counter, Distribution and Gauge). This already exists on DelegatingX classes, but could be done in the base Metric __init__ instead.
   
   Also,
   If it were up to me we would remove Counter, Distribution and Gauge classes and just have the DelegatingCounter DelegatingDistribution and DelegatingGauge classes.
   
   Though, I am unsure if some external implementation to the beam repo uses those classes (Counter, Distribution and Gauge).




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