-
Notifications
You must be signed in to change notification settings - Fork 71
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 granular data to the HTML report graphs #415
Add granular data to the HTML report graphs #415
Conversation
d0ec6c8
to
1f1f6ac
Compare
bd61dc6
to
c2908ac
Compare
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.
I'm very excited to see this coming together! It'll provide much more insight into load test results.
src/graph.rs
Outdated
pub(crate) fn get_tasks_per_second_graph(&self) -> Graph<usize> { | ||
self.create_graph_from_data("graph-tps", "Tasks #", &self.tasks_per_second) | ||
pub(crate) fn get_tasks_per_second_graph(&self) -> Graph<usize, usize> { | ||
self.create_graph_from_single_data("graph-tps", "Tasks #", self.tasks_per_second.clone()) |
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.
Why is tasks_per_second using single_data? It's useful to see more granular data if possible.
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.
Based on what should we divide tasks?
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.
per-task?
src/graph.rs
Outdated
let mut legend = vec!["Total"]; | ||
|
||
let mut other_values = String::new(); | ||
for (title, sub_data) in self.data.iter() { |
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.
Can we sort these by #, so for example the request that makes the most requests in the graph comes first in this legend, the request that makes the second most requests come second, and so on? Otherwise I'm finding it very difficult to match of lines with the legend and to enable/disable specific elements.
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.
(As a reference: this is sorting by name, but likely we can use the same general mechanism to sort by #: https://github.com/tag1consulting/goose/blob/main/src/metrics.rs#L985)
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.
Done.
@@ -331,7 +382,7 @@ impl<'a, T: Serialize> Graph<'a, T> { | |||
var myChart = echarts.init(chartDom); | |||
|
|||
myChart.setOption({{ | |||
color: ['#2c664f'], | |||
color: ['#2c664f', '#5470c6', '#91cc75', '#fac858', '#ee6666', '#73c0de', '#3ba272', '#fc8452', '#9a60b4', '#ea7ccc'], |
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.
So if there are more than 10 elements on the graph, do we re-use the colors?
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.
Correct. We could add more, but at some point they will start repeating anyway.
Maybe there is a way to set these programmatically? A follow-up maybe?
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.
👍 Followup works. Can you create an issue for this?
How difficult would it be to add a flag to disable this verbose data? It creates much larger report files, and perhaps some would prefer the simpler and smaller graphs that we've generated up to this PR? I'd leave granular data enabled by default, but it could be useful to have an option like |
Hello there, just a suggestion after testing your branch, do you think it would be possible to keep the same color for the same request, between two runs? |
32f17fb
to
37babb5
Compare
…data colletion and display.
…time_per_second metric.
…_requests_per_second() and graph::record_errors_per_second().
37babb5
to
7478a19
Compare
7478a19
to
d1a0b6d
Compare
I added
Good point. I added ordering to the lines/legend based on the numbers behind them. I suspect that same order also means same colors so this might solve your suggestion too. @nicompte Do you mind testing a bit? |
7fb42a6
to
0e8ce09
Compare
Works great thanks! |
@@ -59,6 +59,7 @@ const DEFAULT_PORT: &str = "5115"; | |||
/// --no-error-summary Doesn't display an error summary | |||
/// --no-print-metrics Doesn't display metrics at end of load test | |||
/// --report-file NAME Create an html-formatted report | |||
/// --no-granular-data Disables granular data in HTML report graphs |
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.
Would --no-granular-report
make it more clear that this specifically impacts --report-file
and no other data?
/// represents one second. | ||
users_per_second: Vec<usize>, | ||
/// Counts requests per second for each request type. | ||
requests_per_second: HashMap<String, TimeSeries<u32, u32>>, |
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.
(Followup is fine) it seems worthwhile to create a custom Type for HashMap<String, TimeSeries<u32, u32>>
, something like ItemsPerSecond
or PerSecondCounter
or ...? I see it throughout the new code, and we'd document it thoroughly where the type is created.
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 great, thanks! I'm doing more testing, but there's nothing blocking a merge now. I think the configuration option change is worthwhile to be more clear
Closes #410.
I used the default color scheme for now. I don't particularly like it, but I didn't have any better idea either.