Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add transfer_outgoing_bytes_total metric #7388

Merged
merged 2 commits into from
Dec 9, 2022

Conversation

gjoseph92
Copy link
Collaborator

Closes #7123

cc @hendrikmakait @crusaderky @ntabris

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

@ntabris
Copy link
Contributor

ntabris commented Dec 9, 2022

FYI _total will be added to counter metrics so it's not necessary to have in name but also doesn't hurt: https://github.com/prometheus/client_python#counter

gjoseph92 added a commit to gjoseph92/snakebench that referenced this pull request Dec 9, 2022
gjoseph92 added a commit to gjoseph92/snakebench that referenced this pull request Dec 9, 2022
gjoseph92 added a commit to gjoseph92/snakebench that referenced this pull request Dec 9, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2022

Unit Test Results

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

       18 files  ±0         18 suites  ±0   7h 48m 2s ⏱️ - 34m 11s
  3 254 tests ±0    3 166 ✔️ ±0       86 💤 +1  2  - 1 
29 295 runs  ±0  28 067 ✔️ +3  1 226 💤  - 2  2  - 1 

For more details on these failures, see this check.

Results for commit 7249762. ± Comparison against base commit 653b006.

♻️ This comment has been updated with latest results.

distributed/worker.py Outdated Show resolved Hide resolved
Copy link
Member

@hendrikmakait hendrikmakait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gjoseph92, this generally looks good to me. I'd slightly change the wording to clarify what we measure here.

Additionally, I wonder if we should extend this metric with a state label with three states like attempted, succeeded and failed. This would allow us to resolve the issue mentioned in #6936 (comment) and would allow us to drop the corresponding gauge, i.e., requested - (succeeded + failed) = in-progress. I'd leave this for future work though since it can be generalized to all the transfer metrics. (I assume that we are fine with totally breaking changes for new metrics, see #7385.)

@ntabris' nit around _total matches what I propose in #7374, but it doesn't hurt for now.

Co-authored-by: Hendrik Makait <hendrik.makait@gmail.com>
@gjoseph92 gjoseph92 merged commit 047b082 into dask:main Dec 9, 2022
@gjoseph92 gjoseph92 deleted the transfer-outgoing-bytes-total branch December 9, 2022 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

expose transfer bytes total (count) rather than just gauge of current value
3 participants