-
-
Notifications
You must be signed in to change notification settings - Fork 142
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
refac(console): factor out Durations
widget from task view
#408
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There are 2 widgets which display the poll times for a task in the detail view. The poll times percentiles are always displayed and if UTF-8 is available, then a sparkline histogram is also shown to the right. The logic for displaying these two widgets is quite long and is currently interspersed within the `render` function for the task detail view plus helper functions. Additionally, it is not easy to add a second set of widgets showing the time between waking and being polled for a task which is planned for #409. This change factors out that logic into separate widgets. There was already a separate widget `MiniHistogram`. Some of the logic that was previously in the task detail view has been moved here. A new widget `Percentiles` has been added to encapsulate the logic for preparing and displaying the percentiles. A top level `Durations` widget occupies the entire width of the task detail view and control the horizontal layout of the `Percentiles` and `MiniHistogram` widgets. The new widget will also supress the histogram if there isn't at least enough room to display the legend at the bottom.
hawkw
reviewed
Apr 11, 2023
hawkw
approved these changes
Apr 13, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me, thank you!
hawkw
pushed a commit
that referenced
this pull request
Apr 21, 2023
Display the scheduled time percentiles and sparklines for the histogram of scheduled times. The schduled time is the time between when a task is woken and when it is next polled. The scheduled time, which was already calculated, is now stored in a histogram and sent over the wire in together with the task details. This is used to draw percentiles and sparklines on the task details view, in the same way that is done for the poll times histogram. The refactoring done in #408 has been used to more easily display two sets of durations (percentiles and histogram where possible). ## PR Notes The PR depends on both #406, which adds initial support for recording the scheduled (wake-to-poll) time, and #408, which refactors the percentile and histogram widgets to make them easier to reuse. It shouldn't really be reviewed in depth until those two have been merged as it contains a lot of duplication and will need to be rebased. Here are some examples of the scheduled times durations on the task detail view: <img width="1037" alt="task detail view for the sender task in the long-scheduled example" src="https://user-images.githubusercontent.com/89589/232608774-d8ac48a7-3fe7-4742-a75b-e11bdb23abaa.png"> <img width="1043" alt="task detail view for the burn task in the app example" src="https://user-images.githubusercontent.com/89589/232608864-637f4f52-d4a6-468d-88fc-8fe1d53fdff9.png">
hawkw
pushed a commit
that referenced
this pull request
Sep 29, 2023
There are 2 widgets which display the poll times for a task in the detail view. The poll times percentiles are always displayed and if UTF-8 is available, then a sparkline histogram is also shown to the right. The logic for displaying these two widgets is quite long and is currently interspersed within the `render` function for the task detail view plus helper functions. Additionally, it is not easy to add a second set of widgets showing the time between waking and being polled for a task which is planned for #409. This change factors out that logic into separate widgets. There was already a separate widget `MiniHistogram`. Some of the logic that was previously in the task detail view has been moved here. A new widget `Percentiles` has been added to encapsulate the logic for preparing and displaying the percentiles. A top level `Durations` widget occupies the entire width of the task detail view and control the horizontal layout of the `Percentiles` and `MiniHistogram` widgets. The new widget will also supress the histogram if there isn't at least enough room to display the legend at the bottom
hawkw
pushed a commit
that referenced
this pull request
Sep 29, 2023
Display the scheduled time percentiles and sparklines for the histogram of scheduled times. The schduled time is the time between when a task is woken and when it is next polled. The scheduled time, which was already calculated, is now stored in a histogram and sent over the wire in together with the task details. This is used to draw percentiles and sparklines on the task details view, in the same way that is done for the poll times histogram. The refactoring done in #408 has been used to more easily display two sets of durations (percentiles and histogram where possible). ## PR Notes The PR depends on both #406, which adds initial support for recording the scheduled (wake-to-poll) time, and #408, which refactors the percentile and histogram widgets to make them easier to reuse. It shouldn't really be reviewed in depth until those two have been merged as it contains a lot of duplication and will need to be rebased. Here are some examples of the scheduled times durations on the task detail view: <img width="1037" alt="task detail view for the sender task in the long-scheduled example" src="https://user-images.githubusercontent.com/89589/232608774-d8ac48a7-3fe7-4742-a75b-e11bdb23abaa.png"> <img width="1043" alt="task detail view for the burn task in the app example" src="https://user-images.githubusercontent.com/89589/232608864-637f4f52-d4a6-468d-88fc-8fe1d53fdff9.png">
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are 2 widgets which display the poll times for a task in the
detail view. The poll times percentiles are always displayed and if
UTF-8 is available, then a sparkline histogram is also shown to the
right.
The logic for displaying these two widgets is quite long and is
currently interspersed within the
render
function for the task detailview plus helper functions. Additionally, it is not easy to add a second
set of widgets showing the time between waking and being polled for a
task which is planned for #409.
This change factors out that logic into separate widgets.
There was already a separate widget
MiniHistogram
. Some of the logicthat was previously in the task detail view has been moved here.
A new widget
Percentiles
has been added to encapsulate the logic forpreparing and displaying the percentiles.
A top level
Durations
widget occupies the entire width of the taskdetail view and control the horizontal layout of the
Percentiles
andMiniHistogram
widgets. The new widget will also supress the histogramif there isn't at least enough room to display the legend at the bottom.
PR Notes
This refactoring in this PR was heavily inspired by #50, just that the requirements
were a bit different.