gemini-code-assist[bot] commented on code in PR #38749:
URL: https://github.com/apache/beam/pull/38749#discussion_r3328315091
##########
sdks/python/apache_beam/metrics/metric.py:
##########
@@ -231,13 +288,23 @@ class DelegatingStringSet(StringSet):
"""Metrics StringSet that Delegates functionality to MetricsEnvironment."""
def __init__(self, metric_name: MetricName) -> None:
super().__init__(metric_name)
- self.add = MetricUpdater(cells.StringSetCell, metric_name) # type:
ignore[method-assign]
+ self._updater = MetricUpdater(cells.StringSetCell, metric_name)
+
+ def add(self, value: str) -> None:
+ if MetricsFlag.string_set_disabled():
+ return
+ self._updater(value)
class DelegatingBoundedTrie(BoundedTrie):
- """Metrics StringSet that Delegates functionality to MetricsEnvironment."""
+ """Metrics BoundedTrie that Delegates functionality to
MetricsEnvironment."""
def __init__(self, metric_name: MetricName) -> None:
super().__init__(metric_name)
- self.add = MetricUpdater(cells.BoundedTrieCell, metric_name) # type:
ignore[method-assign]
+ self._updater = MetricUpdater(cells.BoundedTrieCell, metric_name)
+
+ def add(self, value) -> None:
+ if MetricsFlag.bounded_trie_disabled():
+ return
+ self._updater(value)
Review Comment:
Access the `string_set_disabled` and `bounded_trie_disabled` class
attributes directly instead of calling classmethod getters to avoid method call
overhead on the hot path.
```suggestion
def add(self, value: str) -> None:
if MetricsFlag.string_set_disabled:
return
self._updater(value)
class DelegatingBoundedTrie(BoundedTrie):
"""Metrics BoundedTrie that Delegates functionality to
MetricsEnvironment."""
def __init__(self, metric_name: MetricName) -> None:
super().__init__(metric_name)
self._updater = MetricUpdater(cells.BoundedTrieCell, metric_name)
def add(self, value) -> None:
if MetricsFlag.bounded_trie_disabled:
return
self._updater(value)
```
##########
sdks/python/apache_beam/metrics/metric.py:
##########
@@ -204,12 +256,17 @@ class DelegatingCounter(Counter):
def __init__(
self, metric_name: MetricName, process_wide: bool = False) -> None:
super().__init__(metric_name)
- self.inc = MetricUpdater( # type: ignore[method-assign]
+ self._updater = MetricUpdater(
cells.CounterCell,
metric_name,
default_value=1,
process_wide=process_wide)
+ def inc(self, n: int = 1) -> None:
+ if MetricsFlag.counter_disabled():
+ return
+ self._updater(n)
Review Comment:
Access the `counter_disabled` class attribute directly instead of calling a
classmethod getter to avoid method call overhead on the hot path.
```suggestion
def inc(self, n: int = 1) -> None:
if MetricsFlag.counter_disabled:
return
self._updater(n)
```
##########
sdks/python/apache_beam/metrics/metric_test.py:
##########
@@ -121,6 +123,78 @@ def test_get_namespace_error(self):
with self.assertRaises(ValueError):
Metrics.get_namespace(object())
+ def test_metrics_flag(self):
+ """Mirrors Java MetricsTest.testMetricsFlag for the three disable*
experiments."""
+ MetricsFlag.reset()
+ self.assertFalse(MetricsFlag.counter_disabled())
+ self.assertFalse(MetricsFlag.string_set_disabled())
+ self.assertFalse(MetricsFlag.bounded_trie_disabled())
+
+ options = PipelineOptions(['--experiments=disableCounterMetrics'])
+ MetricsFlag.set_default_pipeline_options(options)
+ self.assertTrue(MetricsFlag.counter_disabled())
+ self.assertFalse(MetricsFlag.string_set_disabled())
+ self.assertFalse(MetricsFlag.bounded_trie_disabled())
+
+ MetricsFlag.reset()
+ options = PipelineOptions(['--experiments=disableStringSetMetrics'])
+ MetricsFlag.set_default_pipeline_options(options)
+ self.assertFalse(MetricsFlag.counter_disabled())
+ self.assertTrue(MetricsFlag.string_set_disabled())
+ self.assertFalse(MetricsFlag.bounded_trie_disabled())
+
+ MetricsFlag.reset()
+ options = PipelineOptions(['--experiments=disableBoundedTrieMetrics'])
+ MetricsFlag.set_default_pipeline_options(options)
+ self.assertFalse(MetricsFlag.counter_disabled())
+ self.assertFalse(MetricsFlag.string_set_disabled())
+ self.assertTrue(MetricsFlag.bounded_trie_disabled())
+
+ MetricsFlag.reset()
+ options = PipelineOptions([
+ '--experiments=disableCounterMetrics',
+ '--experiments=disableStringSetMetrics',
+ '--experiments=disableBoundedTrieMetrics',
+ ])
+ MetricsFlag.set_default_pipeline_options(options)
+ self.assertTrue(MetricsFlag.counter_disabled())
+ self.assertTrue(MetricsFlag.string_set_disabled())
+ self.assertTrue(MetricsFlag.bounded_trie_disabled())
+
+ MetricsFlag.reset()
Review Comment:
Update the tests to assert on the public class attributes directly instead
of calling the removed getter methods.
```python
def test_metrics_flag(self):
"""Mirrors Java MetricsTest.testMetricsFlag for the three disable*
experiments."""
MetricsFlag.reset()
self.assertFalse(MetricsFlag.counter_disabled)
self.assertFalse(MetricsFlag.string_set_disabled)
self.assertFalse(MetricsFlag.bounded_trie_disabled)
options = PipelineOptions(['--experiments=disableCounterMetrics'])
MetricsFlag.set_default_pipeline_options(options)
self.assertTrue(MetricsFlag.counter_disabled)
self.assertFalse(MetricsFlag.string_set_disabled)
self.assertFalse(MetricsFlag.bounded_trie_disabled)
MetricsFlag.reset()
options = PipelineOptions(['--experiments=disableStringSetMetrics'])
MetricsFlag.set_default_pipeline_options(options)
self.assertFalse(MetricsFlag.counter_disabled)
self.assertTrue(MetricsFlag.string_set_disabled)
self.assertFalse(MetricsFlag.bounded_trie_disabled)
MetricsFlag.reset()
options = PipelineOptions(['--experiments=disableBoundedTrieMetrics'])
MetricsFlag.set_default_pipeline_options(options)
self.assertFalse(MetricsFlag.counter_disabled)
self.assertFalse(MetricsFlag.string_set_disabled)
self.assertTrue(MetricsFlag.bounded_trie_disabled)
MetricsFlag.reset()
options = PipelineOptions([
'--experiments=disableCounterMetrics',
'--experiments=disableStringSetMetrics',
'--experiments=disableBoundedTrieMetrics',
])
MetricsFlag.set_default_pipeline_options(options)
self.assertTrue(MetricsFlag.counter_disabled)
self.assertTrue(MetricsFlag.string_set_disabled)
self.assertTrue(MetricsFlag.bounded_trie_disabled)
MetricsFlag.reset()
```
##########
sdks/python/apache_beam/metrics/metric.py:
##########
@@ -46,18 +46,70 @@
from apache_beam.metrics.metricbase import Histogram
from apache_beam.metrics.metricbase import MetricName
from apache_beam.metrics.metricbase import StringSet
+from apache_beam.options.pipeline_options import DebugOptions
if TYPE_CHECKING:
from apache_beam.internal.metrics.metric import MetricLogger
from apache_beam.metrics.execution import MetricKey
from apache_beam.metrics.metricbase import Metric
+ from apache_beam.options.pipeline_options import PipelineOptions
from apache_beam.utils.histogram import BucketType
__all__ = ['Metrics', 'MetricsFilter', 'Lineage']
_LOGGER = logging.getLogger(__name__)
+class MetricsFlag(object):
+ """Process-wide flags controlling which user metric kinds are emitted.
+
+ Mirrors the Java SDK ``Metrics.MetricsFlag`` behavior. The flags are read
+ once at worker harness initialization from pipeline experiments and apply
+ for the lifetime of the worker.
+ """
+ _counter_disabled = False
+ _string_set_disabled = False
+ _bounded_trie_disabled = False
+ _initialized = False
+
+ @classmethod
+ def set_default_pipeline_options(cls, options: 'PipelineOptions') -> None:
+ """Initialize the flags from pipeline experiments. Idempotent."""
+ if cls._initialized:
+ return
+ debug_options = options.view_as(DebugOptions)
+ if debug_options.lookup_experiment('disableCounterMetrics'):
+ cls._counter_disabled = True
+ _LOGGER.info('Counter metrics are disabled.')
+ if debug_options.lookup_experiment('disableStringSetMetrics'):
+ cls._string_set_disabled = True
+ _LOGGER.info('StringSet metrics are disabled.')
+ if debug_options.lookup_experiment('disableBoundedTrieMetrics'):
+ cls._bounded_trie_disabled = True
+ _LOGGER.info('BoundedTrie metrics are disabled.')
+ cls._initialized = True
+
+ @classmethod
+ def reset(cls) -> None:
+ """Reset flags. Test-only."""
+ cls._counter_disabled = False
+ cls._string_set_disabled = False
+ cls._bounded_trie_disabled = False
+ cls._initialized = False
+
+ @classmethod
+ def counter_disabled(cls) -> bool:
+ return cls._counter_disabled
+
+ @classmethod
+ def string_set_disabled(cls) -> bool:
+ return cls._string_set_disabled
+
+ @classmethod
+ def bounded_trie_disabled(cls) -> bool:
+ return cls._bounded_trie_disabled
Review Comment:
To avoid the overhead of `@classmethod` descriptor lookup and function calls
on the hot path of metric increments, we can simplify `MetricsFlag` by exposing
the disabled flags as public class attributes directly. This aligns with PEP 8
recommendations to avoid redundant getter/setter methods in Python and improves
performance.
```python
counter_disabled = False
string_set_disabled = False
bounded_trie_disabled = False
_initialized = False
@classmethod
def set_default_pipeline_options(cls, options: 'PipelineOptions') -> None:
"""Initialize the flags from pipeline experiments. Idempotent."""
if cls._initialized:
return
debug_options = options.view_as(DebugOptions)
if debug_options.lookup_experiment('disableCounterMetrics'):
cls.counter_disabled = True
_LOGGER.info('Counter metrics are disabled.')
if debug_options.lookup_experiment('disableStringSetMetrics'):
cls.string_set_disabled = True
_LOGGER.info('StringSet metrics are disabled.')
if debug_options.lookup_experiment('disableBoundedTrieMetrics'):
cls.bounded_trie_disabled = True
_LOGGER.info('BoundedTrie metrics are disabled.')
cls._initialized = True
@classmethod
def reset(cls) -> None:
"""Reset flags. Test-only."""
cls.counter_disabled = False
cls.string_set_disabled = False
cls.bounded_trie_disabled = False
cls._initialized = False
```
<details>
<summary>References</summary>
1. PEP 8 recommends avoiding accessor/getter/setter methods for simple
attribute access in Python, preferring direct attribute access instead.
<sup>([link](https://github.com/apache/beam/blob/master/.gemini/styleguide.md))</sup>
</details>
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
