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:
[email protected]
