Styling of charts + only show charts for total stats + clean up & refactoring of charts JS code #539
Conversation
Styled the charts to better match the design of the UI.
I agree ... aggregate stats are the way to go |
locust/static/locust.js
Outdated
| @@ -220,8 +228,13 @@ function initChart(stat) { | |||
| } | |||
|
|
|||
| function updateCharts(stats) { | |||
|
|
|||
| var rsp_chart, rps_data, now, date, avt_chart, avt_data; | |||
There was a problem hiding this comment.
@heyman rsp_chart or rps_chart?
There was a problem hiding this comment.
Oops, typo. I just added those in really quick when I realized we were creating variables in the global scope. Fixed.
locust/static/locust.js
Outdated
|
|
||
| var rps_chart = echarts.init(rps_element.get(0), 'vintage'); | ||
| rps_chart.setOption({ | ||
| title: { | ||
| text: stat.name + ': RPS' | ||
| text: 'RPS', |
There was a problem hiding this comment.
Maybe "Total RPS (Requests per Second)" or "Average RPS (Requests per Second)" or even "Average Req/s." We have plenty of space here so we're not saving anything by abbreviating.
There was a problem hiding this comment.
Changed it to "Total Requests per Second"
locust/static/vintage.js
Outdated
| echarts.registerTheme('vintage', { | ||
| color: colorPalette, | ||
| backgroundColor: '#fef8ef', | ||
| backgroundColor: '#111717', |
There was a problem hiding this comment.
Shouldn't this just be transparent rather than matching the background color?
locust/static/style.css
Outdated
| box-sizing: border-box; | ||
| border: 1px solid #1b1f1f; | ||
| } | ||
| #charts p.note { |
There was a problem hiding this comment.
Minor nitpick but generally tag selectors are discouraged unless style is specifically tied to tag. In this case, the layout could change and lose the style.
|
I really like it! This is the current design for anyone checking later: |
|
I've been using the charting and it's awesome. Two I would add though is the number of users and number of failures tracked over time. Because while this tells an interesting story, it doesn't tell me that the number of users has been increasing during this time and there was actually a point where the failures spiked. It just looks like app performance has been slowly degrading when it's directly correlated with increasing rate of users. |
|
This is looking really cool! |
|
@justiniso Yeah, number of users, and number of failures makes sense. On another note I've also been thinking that we could add a median line, and 95 percentile line to the existing response time chart (and a 95 percentile column to the stats table as well). Something like this, but with a design that matches Locust:
|
heyman
changed the title
This is my quick and dirty styling of the charts to better match Locust's design. There's definitely possible to make them prettier, but at least it's an improvement. We should probably restructure the JS code a bit, like moving out the chart code into it's own JS file (and own JS namespace), but that's for another PR.
I've also changed so that we only render charts for the total stats, and not two graph for every URL entry. The reason for this is that I don't think the value that is added by being able to look at the graph for a specific endpoint (especially in the current ephemeral form of the charts) doesn't justify the huge page that would be generated when running tests with many endpoint. Also it could become a performance issue.
Ping @justiniso