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

feat(console): add wake-to-poll histogram #139

Closed
wants to merge 1 commit into from
Closed

Conversation

sieut
Copy link

@sieut sieut commented Sep 12, 2021

  • refactored MiniHistogram so wake to poll histogram can reuse some
    code from poll duration histogram
  • added Percentiles widget so wake to poll can reuse code from poll
    duration histogram

for #50

- refactored MiniHistogram so wake to poll histogram can reuse some
  code from poll duration histogram
- added Percentiles widget so wake to poll can reuse code from poll
  duration histogram
@sieut
Copy link
Author

sieut commented Sep 12, 2021

This PR is quite long with the refactoring, I can split it into two if that's better for reviewing

@seanmonstar
Copy link
Member

This is excellent work, thanks for getting started here! I think one thing to work out that may affect this PR is whether we want a histogram per task, or if we just want the history of the whole runtime.

I'm tempted to say both sound interesting... The full runtime histogram can help show the overall "health" or busy level. If things are starting to take longer and longer to be polled, it could imply the system has too much load. But knowing at an individual task level could also help understand, when load got high, or another few tasks ate up precious CPU time, that it delayed waking some specific task.

By keeping a log of all the events and timestamps in the "recording", we could always reconstruct either of these. So probably we also want to consider the cost of building these up per tasks and sending them over the wire...

All of my rambling didn't have any decisions, just musings, but hopefully with some joint thinking we'll quickly arrive at a proper balance. :)

@sieut
Copy link
Author

sieut commented Sep 19, 2021

@seanmonstar thank you for the thoughts! I agree with you that recording this data for both runtime and per task is useful here.

So probably we also want to consider the cost of building these up per tasks and sending them over the wire...

This is probably my main concern in keeping logs of events. Given that HdrHistograms are pretty small, another option would be to have another histogram in each TaskStats. What do you think about that idea?

@hawkw hawkw force-pushed the main branch 2 times, most recently from 802ee42 to c3a7660 Compare December 17, 2021 00:56
@sieut sieut requested a review from a team as a code owner May 23, 2022 21:34
@hds
Copy link
Collaborator

hds commented Aug 29, 2023

As this PR was not progressing, a new PR was opened (#409) which implemented the new histogram. That PR has been merged, so I'll close this one.

@hds hds closed this Aug 29, 2023
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.

3 participants