Expose transfer-related metrics in `Worker.get_metrics` and `WorkerMetricCollector` by hendrikmakait · Pull Request #6936 · dask/distributed

hendrikmakait · 2022-08-23T15:34:49Z

Closes #6892

Adds transfer-related metrics in Worker.get_metrics and WorkerMetricCollector
Adds testing for transfer_incoming_* metrics

Out of scope:

Dashboard component for transfer_incoming_bytes and transfer_outgoing_bytes to be added in a follow-up PR

Notes:

outgoing metrics appear to be rather awkward to test since the timing needs to be "just right".

Tests added / passed
Passes pre-commit run --all-files

distributed/http/worker/tests/test_worker_http.py

distributed/http/worker/prometheus/core.py

github-actions · 2022-08-23T16:39:21Z

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

  3 files -       12   3 suites - 12 0s ⏱️ - 6h 37m 28s
21 tests -   3 032 18 ✔️ -   2 950 1 💤 -   83 2 ❌ +1
63 runs - 22 522 54 ✔️ - 21 547 3 💤 - 980 6 ❌ +5

For more details on these failures, see this check.

Results for commit f247b24. ± Comparison against base commit 817ead3.

♻️ This comment has been updated with latest results.

distributed/http/worker/prometheus/core.py

crusaderky · 2022-08-26T11:08:52Z

distributed/worker.py

                "memory": spilled_memory,
                "disk": spilled_disk,
            },
+            comm_reserved_bytes=self.state.comm_nbytes,


Worth noting that on line 967 above,

in_flight=self.state.in_flight_tasks_count,

This is also strictly the number of incoming in-flight tasks.

Outgoing traffic can be measured:

Worker.outgoing_current_count for number of outgoing connections

there is no measure for outgoing number of bytes. However it's straightforward to add it to Worker.get_data:

distributed/distributed/worker.py

Line 1688 in 4cf9baf

nbytes = {k: self.state.tasks[k].nbytes for k in data if k in self.state.tasks}

(add to a total counter after the above line and remove the same amount in a finally clause)

To clarify: it's OK to leave metering of outgoing traffic to a different PR.
However, within the scope of this PR, make it clear through naming that the "in flight tasks" and nbytes are specifically about incoming traffic.

Added transfer_outgoing_bytes

Regarding in_flight: Can we safely rename this?

crusaderky · 2022-08-26T16:11:53Z

@hendrikmakait

To recap our meeting:

rename WorkerState.comm_nbytes to comm_incoming_bytes.
No deprecation in WorkerState needed; just change the already existing DeprecatedWorkerStateAttribute in Worker
new property WorkerState.comm_incoming_count: return len(self.in_flight_workers)
rename Worker.outgoing_current_count to comm_outgoing_count. Add a deprecated property (read-only).
new attribute Worker.comm_outgoing_bytes
rename Worker.incoming_count to comm_incoming_cumulative_count with a deprecated accessor
rename Worker.outgoing_count to comm_outgoing_cumulative_count with a deprecated accessor
heartbeat will now return

comm:
  incoming_bytes: #
  incoming_count: #
  incoming_cumulative_count: #
  outgoing_bytes: #
  outgoing_count: #
  outgoing_cumulative_count: #

update Improve differentiation between incoming/outgoing connections and transfers #6933 to match. Prefer "comm" prefix to "transfers" suffix everywhere.

crusaderky · 2022-08-26T16:21:20Z

[EDIT] let's not drop the cumulative counter; they're free

crusaderky · 2022-08-29T16:00:49Z

Updated naming after conversation on #6933 (comment)
This supersedes my previous post.

main	new	notes
WorkerState.comm_nbytes	WorkerState.transfer_incoming_bytes_current
(new property)	WorkerState.transfer_incoming_count_current	`len(self.in_flight_workers)`
WorkerState.total_out_connections	WorkerState.transfer_incoming_count_max
WorkerState.comm_threshold_bytes	WorkerState.transfer_incoming_bytes_max
WorkerState.target_message_size	(do not change? please discuss)
Worker.outgoing_current_count	Worker.transfer_outgoing_count_current
(new attribute)	Worker.transfer_outgoing_bytes_current
Worker.incoming_count	WorkerState.transfer_incoming_count_cumulative	Moved to WorkerState
Worker.outgoing_count	Worker.transfer_outgoing_count_cumulative
Worker.max_connections	Worker.transfer_outgoing_count_max

Heartbeat will return

transfer:
    incoming_bytes_current: ...
    etc.

crusaderky · 2022-08-29T16:07:29Z

Note that the above list does not include

transfer_incoming_bytes_cumulative
transfer_outgoing_bytes_cumulative

They would be new attributes, but I'd rather leave them for a future PR if and when we realize they're actually useful.
Their meaning would also be non-trivial - are they

the bytes requested, or
the bytes actually received, or
the bytes actually received so far + incoming_current + an adjustment of actual vs. expected at the end of every transfer?

hendrikmakait · 2022-08-29T16:45:02Z

I will WorkerState.comm_threshold_bytes to WorkerState.transfer_incoming_size_throttle_threshold, as it is used as a minimum size of individual transfers to be affected by throttling.

crusaderky · 2022-08-29T19:29:04Z

I will WorkerState.comm_threshold_bytes to WorkerState.transfer_incoming_size_throttle_threshold, as it is used as a minimum size of individual transfers to be affected by throttling.

did you mean transfer_incoming_bytes_throttle_threshold? +1 from me

hendrikmakait · 2022-08-30T09:24:52Z

Regarding WorkerState.target_message_size: We could rename this to WorkerState.transfer_(incoming_)message_target_bytes but I don't have strong opinions on that particular variable.

crusaderky · 2022-08-30T09:46:33Z

Regarding WorkerState.target_message_size: We could rename this to WorkerState.transfer_(incoming_)message_target_bytes but I don't have strong opinions on that particular variable.

this makes sense

hendrikmakait · 2022-08-30T09:50:58Z

Regarding WorkerState.target_message_size: We could rename this to WorkerState.transfer_(incoming_)message_target_bytes but I don't have strong opinions on that particular variable.

this makes sense

Then let's go with WorkerState.transfer_message_target_bytes for brevity as there is currently no conflicting attribute for outgoing messages.

hendrikmakait · 2022-08-30T13:46:16Z

Waiting for #6933 to get merged before giving this a once-over with fresh names.

fjetter · 2022-08-31T09:18:14Z

Waiting for #6936 to get merged before giving this a once-over with fresh names.

Damn deadlocks everywhere...

you're self referencing this issue ;)

hendrikmakait · 2022-08-31T10:08:13Z

Damn deadlocks everywhere...

you're self referencing this issue ;)

I guess we need to start linting comments, I don't see another way.

distributed/worker.py

Co-authored-by: crusaderky <crusaderky@gmail.com>

crusaderky · 2022-08-31T18:28:10Z

ty!

…tricCollector` (dask#6936)

hendrikmakait commented Aug 23, 2022

View reviewed changes

distributed/http/worker/tests/test_worker_http.py Outdated Show resolved Hide resolved

distributed/http/worker/prometheus/core.py Outdated Show resolved Hide resolved

hendrikmakait marked this pull request as ready for review August 24, 2022 10:03

hendrikmakait requested review from crusaderky, fjetter, gjoseph92 and graingert August 24, 2022 10:03

hendrikmakait self-assigned this Aug 25, 2022

crusaderky reviewed Aug 26, 2022

View reviewed changes

distributed/http/worker/prometheus/core.py Outdated Show resolved Hide resolved

crusaderky reviewed Aug 26, 2022

View reviewed changes

crusaderky mentioned this pull request Aug 26, 2022

Improve differentiation between incoming/outgoing connections and transfers #6933

Merged

2 tasks

hendrikmakait force-pushed the add-comms-metrics branch from ff3ae35 to f247b24 Compare August 30, 2022 19:35

hendrikmakait marked this pull request as draft August 31, 2022 07:48

hendrikmakait changed the title ~~Expose comm_nbytes in Worker.get_metrics and WorkerMetricCollector~~ Expose transfer-related metrics in Worker.get_metrics and WorkerMetricCollector Aug 31, 2022

hendrikmakait force-pushed the add-comms-metrics branch from 98ba29c to 15dce70 Compare August 31, 2022 11:41

hendrikmakait marked this pull request as ready for review August 31, 2022 11:45

hendrikmakait mentioned this pull request Aug 31, 2022

Add dashboard component for size of open data transfers #6982

Merged

2 tasks

hendrikmakait requested a review from crusaderky August 31, 2022 15:52

hendrikmakait added 5 commits August 31, 2022 18:24

Expose comm_nbytes in get_metrics and prometheus

b09570b

Adjust total count logic and calculate transfer_outgoing_bytes

aba58f0

Fix test

1dce893

Test transfer_incoming metrics

bf4c97f

Revert nbytes default

efe0a62

hendrikmakait force-pushed the add-comms-metrics branch from 7d28792 to efe0a62 Compare August 31, 2022 16:24

crusaderky requested changes Aug 31, 2022

View reviewed changes

distributed/worker.py Outdated Show resolved Hide resolved

distributed/worker.py Outdated Show resolved Hide resolved

distributed/worker.py Outdated Show resolved Hide resolved

hendrikmakait and others added 3 commits August 31, 2022 18:52

Apply suggestions from code review

8805a49

Co-authored-by: crusaderky <crusaderky@gmail.com>

Align docstrings

67a01ce

Minor

d6b3fd7

crusaderky approved these changes Aug 31, 2022

View reviewed changes

crusaderky merged commit bfc5cfe into dask:main Aug 31, 2022

hendrikmakait mentioned this pull request Oct 17, 2022

expose transfer bytes total (count) rather than just gauge of current value #7123

Closed

fjetter mentioned this pull request Oct 18, 2022

bokeh and tornado: KeyError: 'transfer' #7133

Open

gjoseph92 pushed a commit to gjoseph92/distributed that referenced this pull request Oct 31, 2022

Expose transfer-related metrics in Worker.get_metrics and `WorkerMe…

6a8708c

…tricCollector` (dask#6936)

Uh oh!

Conversation

hendrikmakait commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

Uh oh!

crusaderky Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

crusaderky Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

crusaderky Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

hendrikmakait Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

hendrikmakait Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

crusaderky commented Aug 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crusaderky commented Aug 26, 2022

Uh oh!

crusaderky commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crusaderky commented Aug 29, 2022

Uh oh!

hendrikmakait commented Aug 29, 2022

Uh oh!

crusaderky commented Aug 29, 2022

Uh oh!

hendrikmakait commented Aug 30, 2022

Uh oh!

crusaderky commented Aug 30, 2022

Uh oh!

hendrikmakait commented Aug 30, 2022

Uh oh!

hendrikmakait commented Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjetter commented Aug 31, 2022

Uh oh!

hendrikmakait commented Aug 31, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

crusaderky commented Aug 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hendrikmakait commented Aug 23, 2022 •

edited

Loading

github-actions bot commented Aug 23, 2022 •

edited

Loading

crusaderky commented Aug 26, 2022 •

edited

Loading

crusaderky commented Aug 29, 2022 •

edited

Loading

hendrikmakait commented Aug 30, 2022 •

edited

Loading