fullstorydev / solr-bench

Solr benchmarking and load testing harness
Apache License 2.0
17 stars 10 forks source link

Unified graphs #52

Closed chatman closed 1 year ago

chatman commented 1 year ago

Currently, the open source graphs (http://mostly.cool) are using a different generation logic (based on createBranchGraphs.py). Intention here is to port those graphs over to the graph-scripts/ logic. However, there are a few gaps in functionality there, which I'm trying to address:

  1. We need support for plotting multiple tests at the same time (across multiple branches).
  2. A single test run results in multiple graphs (one for each task). We should be able to combine them into a single graph. Motivation for this usecase is that going forward, there might be tens/hundreds of tests added each with multiple tasks, so having one graph per test (for a given branch) would make it more readable when scanning through for regressions.

Here's the first step towards that:

chatman commented 1 year ago

Here's an example of two tests with this (attached ZIP file contains generated graph-data.js):

python3 graph-scripts/generate_graph_json.py -r suites/results/stress-facets-local -r suites/results/cluster-test -b branch_9x...branch_9_1

graph.zip

chatman commented 1 year ago

I've published the graphs using the latest commit to http://mostly.cool/graph.html.

Click "branch_9x", then "Collapse" button on top. Here's how it looks for me: image

chatman commented 1 year ago

TODO: bring back the task descriptions, possibly even link the suites on GH.

chatman commented 1 year ago

@patsonluk @justinrsweeney @noblepaul When you please get a chance, can you weigh in on this idea (collapsed vs. expanded)? Apart from the TODO mentioned above, and also maybe deciding which one should be default (expanded or collapsed), this PR is ready for your review/merge. Thanks!

To play around with this live, check http://mostly.cool/graph.html

justinrsweeney commented 1 year ago

My two cents is that we should default to collapsed. I think it provides a more concise overall view and users can expand for additional details.

patsonluk commented 1 year ago

@chatman Thanks for the proposal! Just want to ensure that I understand the motivation correctly:

We need support for plotting multiple tests at the same time (across multiple branches).

So for example someone can have a test suite that does only queries, and another test suite only indexing. And we want to see all the results within the same report (ie html file)?

A single test run results in multiple graphs (one for each task).

This is the current implementation before this proposed change as I understand it :) I actually broke it up this way as in our scenario, having all tasks within the same graph (restart/index/query) didn't make too much sense - as restart has very high duration with makes the line for querying (median per query) almost invisible.

We should be able to combine them into a single graph. Motivation for this usecase is that going forward, there might be tens/hundreds of tests added each with multiple tasks, so having one graph per test (for a given branch) would make it more readable when scanning through for regressions.

I assume Per test refers to test template/suite. not the test runs themselves? So displaying multiple tasks within the same test suite on a single graph? I assume the motivation is to lower the number of graphs generated (vs one graph per task) 😊 .

If that's the case, then this might make sense if all the tasks have similar durations. Otherwise having tasks with median of much longer duration might hide anomaly of tasks with short durations.

chatman commented 1 year ago

Sorry for missing your comment here! :-( Hence the late reply.

So for example someone can have a test suite that does only queries, and another test suite only indexing. And we want to see all the results within the same report (ie html file)?

Yes

This is the current implementation before this proposed change as I understand it :) I actually broke it up this way as in our scenario, having all tasks within the same graph (restart/index/query) didn't make too much sense - as restart has very high duration with makes the line for querying (median per query) almost invisible.

Yes, that makes sense. Stuffing all the different tasks into a single graph (esp with different units at times) can be confusing, and separate graphs per task makes sense. However, it is good in cases where the numbers of test suites themselves are few. In open source graphs (mostly.cool/graph.html at the moment), there will conceivably be 100s of test suites (testing various parts of Solr's functionality), hence introduced this "collapsed" mode.

I assume Per test refers to test template/suite. not the test runs themselves? So displaying multiple tasks within the same test suite on a single graph? I assume the motivation is to lower the number of graphs generated (vs one graph per task) blush .

Right, "per test" == "per test suite". Exactly, you're right about the motivation.

If that's the case, then this might make sense if all the tasks have similar durations. Otherwise having tasks with median of much longer duration might hide anomaly of tasks with short durations.

Yes, you're right. To mitigate that (surface the anamolies of shorter tasks better), I introduced the click to hide/show tasks in the legend. Not ideal, but hopefully gets the job done manually by hiding the longer duration tasks and only showing the shorter ones, when inspecting the graphs.

chatman commented 1 year ago

My two cents is that we should default to collapsed. I think it provides a more concise overall view and users can expand for additional details.

I've implemented this. All single group tabs now have collapsed graphs by default, and the "Expand" button works by showing graphs on a per task basis, as Patson implemented them. WDYT, @patsonluk . In case you have an objection to making this default, I can work on adding some configuration parameter for collapsed vs expanded by default to the python based generation script, and pass it through to the JS.

chatman commented 1 year ago

Merging this for progress. One thing we still need to do (perhaps in a separate ticket) is to bring back the descriptions of the tasks from the config files to be shown on the graphs.

patsonluk commented 1 year ago

Merging this for progress. One thing we still need to do (perhaps in a separate ticket) is to bring back the descriptions of the tasks from the config files to be shown on the graphs.

Thanks @chatman ! I will merge my PR as well and try it out!

patsonluk commented 1 year ago

@chatman I have added an optional param expand to the graph.html such that it should expand/collapse on load

https://github.com/fullstorydev/solr-bench/commit/990051eebb04610ff9ee3c57fe15a9edcdd3a69b

By default it is false, so it would preserve ur behavior. As for FS usage, we will probably prompt user to use graph/graph.html?expand=true

chatman commented 1 year ago

Sounds perfect. Thanks!

On Tue, 28 Feb, 2023, 2:32 am patsonluk, @.***> wrote:

@chatman https://github.com/chatman I have added an optional param expand to the graph.html such that it should expand/collapse on load

990051e https://github.com/fullstorydev/solr-bench/commit/990051eebb04610ff9ee3c57fe15a9edcdd3a69b

By default it is false, so it would preserve ur behavior. As for FS usage, we will probably prompt user to use graph/graph.html?expand=true

— Reply to this email directly, view it on GitHub https://github.com/fullstorydev/solr-bench/pull/52#issuecomment-1447083065, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABDCR5A3HRXVPNRYL5ZUE3DWZUI45ANCNFSM6AAAAAAVA2RAWQ . You are receiving this because you were mentioned.Message ID: @.***>