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.

Sign up for GitHub

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

Jump to bottom

Post fine performance metrics to Spans #7885

Merged
merged 1 commit into from
Jun 8, 2023
Merged

Post fine performance metrics to Spans #7885

merged 1 commit into from
Jun 8, 2023

Conversation

crusaderky
Copy link
Collaborator

@crusaderky crusaderky commented Jun 6, 2023 •
edited
Loading

Out of scope: make the Bokeh GUI span-aware

@crusaderky crusaderky self-assigned this Jun 6, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023 •
edited
Loading

Unit Test Results

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

       20 files  ±  0         20 suites  ±0   12h 6m 36s ⏱️ + 15m 27s
  3 679 tests +  4    3 565 ✔️ +  2     108 💤 ±0    6 ❌ +  2 
35 580 runs  +40  33 801 ✔️ +29  1 765 💤 +1  14 ❌ +10 

For more details on these failures, see this check.

Results for commit f7721e1. ± Comparison against base commit e31c864.

This pull request removes 1 and adds 5 tests. Note that renamed tests count towards both. 
distributed.tests.test_scheduler ‑ test_cumulative_worker_metrics

distributed.tests.test_spans ‑ test_worker_metrics
distributed.tests.test_worker_metrics ‑ test_no_spans_extension
distributed.tests.test_worker_metrics ‑ test_reschedule
distributed.tests.test_worker_metrics ‑ test_send_metrics_to_scheduler
distributed.tests.test_worker_metrics ‑ test_user_metrics_weird

♻️ This comment has been updated with latest results.

@crusaderky crusaderky force-pushed the spans/metrics branch 5 times, most recently from 7b9b21a to 74a4a07 Compare June 7, 2023 14:33
crusaderky
crusaderky commented Jun 7, 2023
View reviewed changes
distributed/dashboard/components/scheduler.py Outdated
Comment on lines 3424 to 3432
items = defaultdict(float)
for k, v in self.scheduler.cumulative_worker_metrics.items():
if isinstance(k, tuple) and k[0] == "get-data":
items[k] = v
elif isinstance(k, tuple) and k[0] == "execute":
items[k[:1] + k[2:]] += v # sum all span_id's together
# Note: this sort works because we removed span_id's. Otherwise, it would crash
# when the spans extension is disabled and span_id's are None as a consequence.
items = sorted(items.items())
Copy link
Collaborator Author

@crusaderky crusaderky Jun 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

FYI @milesgranger
Support in bokeh for the spans is highly desirable but I'd rather leave it to a different PR.
XREF #7889

@crusaderky crusaderky mentioned this pull request Jun 7, 2023
Closed
crusaderky
crusaderky commented Jun 7, 2023
View reviewed changes
distributed/tests/test_scheduler.py Outdated

assert all(isinstance(value, float) for value in metrics.values())


Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moved to test_worker_metrics.py and overhauled

crusaderky
crusaderky commented Jun 7, 2023
View reviewed changes
distributed/worker_state_machine.py
self,
task_name: str,
func: Callable[P, Awaitable[StateMachineEvent]],
/,
*args: P.args,
span_id: str | None = None,
Copy link
Collaborator Author

@crusaderky crusaderky Jun 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

By the time the async instruction finishes, the task may have been forgotten, so we need to preserve the span_id. If we didn't, time wasted by work stealing would quietly disappear from all span-specific metrics.

@crusaderky crusaderky marked this pull request as ready for review June 7, 2023 16:06
@crusaderky crusaderky requested a review from fjetter as a code owner June 7, 2023 16:06
@crusaderky crusaderky requested a review from hendrikmakait June 7, 2023 16:06
@crusaderky crusaderky mentioned this pull request Jun 7, 2023
Closed
@crusaderky
Copy link
Collaborator Author

A test is failing; investigating

  File "/Users/runner/work/distributed/distributed/distributed/spans.py", line 423, in heartbeat
    _, span_id, prefix, activity, unit = k
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
ValueError: too many values to unpack (expected 5)

milesgranger
milesgranger approved these changes Jun 8, 2023
View reviewed changes
Copy link
Contributor

@milesgranger milesgranger left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Overall seems good to me, barring the test failure you're already looking into.

distributed/spans.py
Comment on lines +274 to +277
At the moment of writing, all keys are
``("execute", <task prefix>, <activity>, <unit>)``
but more may be added in the future with a different format; please test for
``k[0] == "execute"``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it'd be possible later on to map everything to some dataclass or similar? There's an awful lot of tedious checks building up for key lengths / values to determine what's what.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it is certainly possible and sensible, yes

@crusaderky
Copy link
Collaborator Author

crusaderky commented Jun 8, 2023 •
edited
Loading

This is now blocked by and built on top #7894
[EDIT] merged

@crusaderky crusaderky marked this pull request as draft June 8, 2023 13:17
@crusaderky crusaderky marked this pull request as ready for review June 8, 2023 13:20
@crusaderky
Copy link
Collaborator Author

This is ready for final review. @milesgranger could you re-review distributed/dashboard/components/scheduler.py? I just did major changes to it.

milesgranger
milesgranger approved these changes Jun 8, 2023
View reviewed changes
Copy link
Contributor

@milesgranger milesgranger left a comment

Choose a reason for hiding this comment

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

The overhaul w/ get_metrics is an improvement IMO, thanks!

@crusaderky crusaderky merged commit 3ab53fc into dask:main Jun 8, 2023
@crusaderky crusaderky deleted the spans/metrics branch June 8, 2023 15:14
@crusaderky crusaderky mentioned this pull request Jun 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@milesgranger milesgranger milesgranger approved these changes

@fjetter fjetter Awaiting requested review from fjetter fjetter is a code owner

@hendrikmakait hendrikmakait Awaiting requested review from hendrikmakait

Assignees

@crusaderky crusaderky

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fine performance metrics: apportion to Computations
2 participants
@crusaderky @milesgranger