-
Notifications
You must be signed in to change notification settings - Fork 153
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 history graph to benchmark detail #1690
Conversation
301db8a
to
a4b19c4
Compare
I haven't looked at the code yet, but I don't understand the client side cache - isn't each graph on the compare page distinct? |
By default, Vue components are destroyed when they stop being rendered. This means that if you would "spam" clicking on the arrow for the same benchmark row, it would reload the graph from the server on each display of the detail component. This is what the client side cache prevents. |
I think that's fine for now then. We only have a server side cache for the index.html default graphs endpoint, and I'm not even sure how critical that is. |
a4b19c4
to
cf17be0
Compare
I added a comment to the code to explain why the client side cache is there. |
Changed the graph kind based on @lqd's suggestion. |
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.
Sweet!
site/frontend/src/pages/compare/compile/table/benchmark-detail.vue
Outdated
Show resolved
Hide resolved
Would it be possible to somehow detect wether there are future results for the given test cases and show future results? This can be useful for determining noise. If the change is not reversed in future results, it's easier to determine it might be noise. |
For triage purposes I also have to do this a lot, and had already asked if this feature could e.g. show up to the next 10 commits following the end range, to see whether it was a sustained change or things returned to steady state. I believe the short answer is yes, but maybe not in this PR per se. |
Showing future commits is a good idea, I wanted to do it in a future PR, but I guess that if we find a reasonable way to implement, we can include it here outright. So, how should it work? Some ideas:
|
Honestly, just having some future data would already be awesome, so whichever way is the easiest to do. And we can iterate in the future, once we have experience with it. Ryan/Mark/Felix may have a different process, but some thoughts:
|
8fc6953
to
f92b547
Compare
I have another PR prepared on top of this one, which adds links to the benchmark detail, I'll add this to that PR. |
I personally think 30 days is too long, but I don't want to block this PR. We can always adjust timeframes in the future. Thanks for working on this! |
FWIW we recently had a case in rust-lang/rust#114492 with similar noisy behavior that had happened 15 days in the past. edit: and which would have looked like this |
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.
This is going to be super helpful!
I'll look into the background-color regions thingy I mentioned, there's no need to block this PR: the dashed vertical line can collide/hide the graphed data itself.
(I can't easily test the behavior on a try build since that requires server-side changes, so rust-lang/rust#102099 (comment) understandably shows up locally as a real merge, with future data and dashed line)
If there are tiny issues, we can fix them.
This PR adds a mini 30 day history graph to the benchmark detail component introduced in #1689. This should help us quickly access the history of a specific benchmark.
Loading of these graphs is cached on the client side, i.e. the same graph will only be loaded once (on-demand) per page load. @Mark-Simulacrum If you're worried about the server load that could be caused by this, I can also add a cache on the server side.
graph.mp4
I didn't use the single graph endpoint introduced in #1054, because it doesn't return the commit list, and thus the response cannot be directly used with the existing code to render a graph. And since the
/graphs
endpoint can actually return just a single graph, I think that the single graph endpoint is basically redundant at this point.