Filter empty distribution metrics #32027

Naireen · 2024-07-30T22:05:32Z

Filter empty distributions

On the python sdk, filter emtpy distributions so its not sent to the runner for processing, since it adds unnecessary overhead. This can occur when processing failed, or there was an empty split created (for batch pipelines)

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

github-actions · 2024-07-31T00:49:17Z

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @tvalentyn for label python.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

Naireen · 2024-07-31T00:58:06Z

Run Python_Coverage PreCommit

sdks/python/apache_beam/runners/worker/bundle_processor.py

tvalentyn · 2024-08-09T17:14:11Z

sdks/python/apache_beam/runners/worker/operations.py

-    all_monitoring_infos[monitoring_infos.to_key(
-        sampled_byte_count)] = sampled_byte_count
+
+    try:


would it make sense to check here if(count > 0) instead?

tvalentyn · 2024-08-09T17:17:17Z

sdks/python/apache_beam/metrics/monitoring_infos.py

@@ -214,6 +214,11 @@ def int64_user_distribution(namespace, name, metric, ptransform=None):
    ptransform: The ptransform id used as a label.
  """
  labels = create_labels(ptransform=ptransform, namespace=namespace, name=name)
+  if metric.count <= 0:
+    raise TypeError(


I am not seeing where this exception will be caught, hence same question - can we avoid entering this codepath instead of catching the exception in a try-catch.

ValueError would be more appropriate here.

Also if exception is uncaught, it can create a breaking change for users, so as much as possible I'd prefer to not enter the exception path or fail silently

For the Dataflow Runner, there are checks when we create the distribution, so that should be fine. For a user defined counter, what behaviour do you want? I agree we shouldn't introduce breaking changes. Is it fine to not emit a counter in that case? If we still want to emit something here, then we'll have an empty counter, and we'd have to filter it out in the runner to prevent sending it to the backend (which we don't want to add either based on your previous comments.)

I think not emitting sounds fine, left some comments.

tvalentyn · 2024-08-09T17:17:35Z

sdks/python/apache_beam/metrics/monitoring_infos.py

+  if metric.count <= 0:
+    raise TypeError(
+        'Expected a non zero distribution count for %s metric but received %s' %
+        (metric, metric.count))


you probably meant metric.name here

tvalentyn · 2024-08-12T22:06:26Z

sdks/python/apache_beam/metrics/monitoring_infos.py

-      coders.VarIntCoder(), metric.count, metric.sum, metric.min, metric.max)
-  return create_monitoring_info(
-      USER_DISTRIBUTION_URN, DISTRIBUTION_INT64_TYPE, payload, labels)
+  if metric.count <= 0:


nit: I'd prefer to lead the condition with the happy path (if metric.count > 0)

tvalentyn · 2024-08-12T22:10:56Z

sdks/python/apache_beam/metrics/monitoring_infos.py

+    _LOGGER.debug(
+        'Expected a non zero distribution count for %s metric but received %s' %
+        (metric.name, metric.count))
+    return


Since we return None (implicitly), we need to change the method typehint to # type: (...) -> Optional[metrics_pb2.MonitoringInfo].

tvalentyn · 2024-08-12T22:21:27Z

sdks/python/apache_beam/metrics/monitoring_infos.py

-      USER_DISTRIBUTION_URN, DISTRIBUTION_INT64_TYPE, payload, labels)
+  if metric.count <= 0:
+    _LOGGER.debug(
+        'Expected a non zero distribution count for %s metric but received %s' %


In which situation this log message will be actionable? I wonder if we should remove this log if it commonly happens (e.g. retries).

tvalentyn · 2024-08-12T22:22:10Z

sdks/python/apache_beam/runners/worker/operations.py

@@ -605,18 +605,23 @@ def pcollection_count_monitoring_infos(self, tag_to_pcollection_id):
        receiver.opcounter.element_counter.value(),
        pcollection=pcollection_id,
    )
+    all_monitoring_infos[monitoring_infos.to_key(elem_count_mi)] = elem_count_mi


Do we need to check that elem_count_mi is not None here?

Also, what happens with distribution if the count is 0 after all? For example , pipeline reads from a file, but there are no elements, and pipeline stops. will this case be handled correctly?

tvalentyn · 2024-08-12T22:26:46Z

sdks/python/apache_beam/metrics/monitoring_infos.py

@@ -214,6 +214,11 @@ def int64_user_distribution(namespace, name, metric, ptransform=None):
    ptransform: The ptransform id used as a label.
  """
  labels = create_labels(ptransform=ptransform, namespace=namespace, name=name)
+  if metric.count <= 0:
+    raise TypeError(


I think not emitting sounds fine, left some comments.

Naireen · 2024-08-12T22:28:24Z

So with the current approach of returning None, this fails:

./gradlew :sdks:python:test-suites:tox:pycommon:mypy (part of the Precommig Python Lint test)

Essentially the issue is that we have a dict of strings input, which should produce a dict of Monitoring info. Since we aren't raising errors, pyhint fails since it now expects monitoring_infos.int64_distribution changes from metrics_pb2.MonitoringInfo to Optional[metrics_pb2.MonitoringInfo], which makes operations.pcollection_count_monitoring_infos invalid, can't go from Dict[FrozenSet, metrics_pb2.MonitoringInfo] to Dict[FrozenSet, Optional[metrics_pb2.MonitoringInfo]]

Ideally we do some filtering here, if we went down this approach, which isn't the most performant

The other option was to raise an error, gaurd the runner against it, so then if a user creates a invalid distribution, it would error (which is a breaking change, which also isn't desirable)

Then which of the two approaches make sense? Is there a third option?

github-actions · 2024-08-20T14:47:04Z

Reminder, please take a look at this pr: @tvalentyn

tvalentyn · 2024-08-20T16:36:52Z

Discussed offline

tvalentyn · 2024-08-20T16:37:00Z

waiting on author

github-actions · 2024-10-20T12:44:40Z

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

github-actions · 2024-10-28T12:46:05Z

This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

github-actions bot added the python label Jul 30, 2024

Naireen force-pushed the fix_distribution_counter branch 5 times, most recently from afc0bb3 to b0d332d Compare July 30, 2024 22:14

Naireen marked this pull request as ready for review July 30, 2024 22:15

Naireen force-pushed the fix_distribution_counter branch from b0d332d to 4a18297 Compare July 30, 2024 23:18

github-actions bot added the Next Action: Reviewers label Jul 31, 2024

Filter empty distribution metrics

c92ba74

Naireen force-pushed the fix_distribution_counter branch from 4a18297 to c92ba74 Compare July 31, 2024 16:49

tvalentyn reviewed Jul 31, 2024

View reviewed changes

sdks/python/apache_beam/runners/worker/bundle_processor.py Outdated Show resolved Hide resolved

Naireen force-pushed the fix_distribution_counter branch 6 times, most recently from e17a6fa to d1eee16 Compare August 1, 2024 22:14

test changes

e84f81d

Naireen force-pushed the fix_distribution_counter branch from d1eee16 to e84f81d Compare August 2, 2024 17:25

tvalentyn reviewed Aug 9, 2024

View reviewed changes

Naireen force-pushed the fix_distribution_counter branch 2 times, most recently from 40d4591 to cf01c43 Compare August 12, 2024 15:48

Address review comments

86eff84

Naireen force-pushed the fix_distribution_counter branch from cf01c43 to 86eff84 Compare August 12, 2024 17:09

tvalentyn reviewed Aug 12, 2024

View reviewed changes

github-actions bot added the slow-review label Aug 20, 2024

github-actions bot added slow-review Next Action: Author and removed slow-review Next Action: Reviewers labels Aug 20, 2024

github-actions bot added the stale label Oct 20, 2024

github-actions bot closed this Oct 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter empty distribution metrics #32027

Filter empty distribution metrics #32027

Naireen commented Jul 30, 2024 •

edited

Loading

github-actions bot commented Jul 31, 2024

Naireen commented Jul 31, 2024

tvalentyn Aug 9, 2024

Naireen Aug 12, 2024

tvalentyn Aug 9, 2024

tvalentyn Aug 9, 2024

Naireen Aug 12, 2024

tvalentyn Aug 12, 2024

tvalentyn Aug 9, 2024

Naireen Aug 12, 2024

tvalentyn Aug 12, 2024

tvalentyn Aug 12, 2024

tvalentyn Aug 12, 2024

tvalentyn Aug 12, 2024

tvalentyn Aug 12, 2024

tvalentyn Aug 12, 2024

Naireen commented Aug 12, 2024

github-actions bot commented Aug 20, 2024

tvalentyn commented Aug 20, 2024

tvalentyn commented Aug 20, 2024

github-actions bot commented Oct 20, 2024

github-actions bot commented Oct 28, 2024

Filter empty distribution metrics #32027

Filter empty distribution metrics #32027

Conversation

Naireen commented Jul 30, 2024 • edited Loading

GitHub Actions Tests Status (on master branch)

github-actions bot commented Jul 31, 2024

Naireen commented Jul 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naireen commented Aug 12, 2024

github-actions bot commented Aug 20, 2024

tvalentyn commented Aug 20, 2024

tvalentyn commented Aug 20, 2024

github-actions bot commented Oct 20, 2024

github-actions bot commented Oct 28, 2024

Naireen commented Jul 30, 2024 •

edited

Loading