goose icon indicating copy to clipboard operation
goose copied to clipboard

Adding baseline to reports

Open ctron opened this issue 1 year ago • 11 comments

Based on #600

ctron avatar Aug 01 '24 07:08 ctron

This new feature would benefit from some documentation. It's not clear to me how to use it. Is this a bug, or am I doing something wrong?

 % file report.json
report.json: JSON data
 % cargo run --release --example umami -- --report-file report.html --report-file report.md --baseline-file report.json --host http://d11-r 5 -t 10s
    Finished `release` profile [optimized] target(s) in 0.11s
     Running `target/release/examples/umami --report-file report.html --report-file report.md --baseline-file report.json --host 'http://d11.pozza' -r 5 -t 10s`
12:55:38 [INFO] Output verbosity level: INFO
12:55:38 [INFO] Logfile verbosity level: WARN
12:55:38 [INFO] users defaulted to number of CPUs = 16
12:55:38 [INFO] run_time = 10
12:55:38 [INFO] hatch_rate = 5
12:55:38 [INFO] report_file = ["report.html", "report.md"]
12:55:38 [INFO] baseline_file = report.json
12:55:38 [INFO] iterations = 0
12:55:38 [INFO] global host configured: http://d11
12:55:38 [INFO] allocating transactions and scenarios with RoundRobin scheduler
12:55:38 [INFO] initializing 16 user states...
12:55:38 [INFO] WebSocket controller listening on: 0.0.0.0:5117
12:55:38 [INFO] Telnet controller listening on: 0.0.0.0:5116
Error: Serde(Error("data did not match any variant of untagged enum Value", line: 1764, column: 36))

jeremyandrews avatar Aug 28 '24 12:08 jeremyandrews

This new feature would benefit from some documentation.

Yes, I can understand that :rofl: … I wanted to wait for the other PR to finish first. I did rebase and update this PR now, also brought back the "number format" for big integers. I'll work on documentation next.

ctron avatar Aug 28 '24 13:08 ctron

I am able to reproduce this. I looks like these are cases where a division by zero happens, which results in f32 being NaN, which gets serializes null. Causing errors during loading. I'll take a look how we can solve this.

ctron avatar Aug 28 '24 13:08 ctron

The problem seems to be this:

fn main() {
    let json = serde_json::to_string(&(0f32 / 0f32)).unwrap();
    println!("{json}");
    let value : f32 = serde_json::from_str(&json).unwrap();
    println!("{value}");
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9f980adb4c79dfe7ca872497beb7f6c9

ctron avatar Aug 28 '24 13:08 ctron

And here is the corresponding issue: https://github.com/serde-rs/json/issues/202 … It might make sense to follow the way of adding a custom deserializer: https://github.com/serde-rs/json/issues/202#issuecomment-1925395889

ctron avatar Aug 28 '24 13:08 ctron

I updated the PR. Works for me now. Still lacks documentation.

ctron avatar Aug 28 '24 14:08 ctron

 % git pull --rebase
remote: Enumerating objects: 29, done.
remote: Counting objects: 100% (29/29), done.
remote: Compressing objects: 100% (13/13), done.
remote: Total 29 (delta 15), reused 29 (delta 15), pack-reused 0 (from 0)
Unpacking objects: 100% (29/29), 11.61 KiB | 625.00 KiB/s, done.
From github.com:ctron/goose
 * branch            feature/baseline_1 -> FETCH_HEAD
Updating 362de7d..d5ae6ae
Fast-forward
 src/config.rs                                      |   4 +-
 src/docs/goose-book/src/getting-started/metrics.md |  12 +++
 .../src/getting-started/runtime-options.md         |   1 +
 src/metrics.rs                                     |  12 ++-
 src/metrics/common.rs                              |   5 +-
 src/metrics/delta.rs                               | 103 ++++++++++++++++++---
 src/metrics/nullable.rs                            |  58 ++++++++++++
 src/report.rs                                      |  66 ++++++-------
 src/report/common.rs                               |  25 ++++-
 src/report/markdown.rs                             |   9 ++
 10 files changed, 240 insertions(+), 55 deletions(-)
 create mode 100644 src/metrics/nullable.rs
 % cargo run --release --example umami -- --report-file report.html --report-file report.md --baseline-file report.json --host http://d11-r 5 -t 10s
   Compiling goose v0.17.3-dev (~/goose)
error[E0658]: associated type bounds are unstable
  --> src/metrics/delta.rs:99:19
   |
99 |     T: DeltaValue<Delta: ToFormattedString> + ToFormattedString,
   |                   ^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: see issue #52662 <https://github.com/rust-lang/rust/issues/52662> for more information

error[E0658]: associated type bounds are unstable
    --> src/metrics.rs:3371:19
     |
3371 |     T: DeltaValue<Delta: ToFormattedString> + ToFormattedString,
     |                   ^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: see issue #52662 <https://github.com/rust-lang/rust/issues/52662> for more information

For more information about this error, try `rustc --explain E0658`.
error: could not compile `goose` (lib) due to 2 previous errors
 % git status
On branch feature/baseline_1
Untracked files:
  (use "git add <file>..." to include in what will be committed)
	report.html
	report.json
	report.md

nothing added to commit but untracked files present (use "git add" to track)
 % git log --pretty=format:'%h' -n 1
d5ae6ae

jeremyandrews avatar Aug 28 '24 14:08 jeremyandrews

Ah, that was stabilized in 1.79.0. IIRC that was only syntactic sugar. I'll see if I can work around that.

ctron avatar Aug 28 '24 14:08 ctron

Ah, that was stabilized in 1.79.0. IIRC that was only syntactic sugar. I'll see if I can work around that.

Done.

ctron avatar Aug 28 '24 14:08 ctron

LGTM (and yes I would say that!)

JimFuller-RedHat avatar Sep 03 '24 11:09 JimFuller-RedHat

I just noticed a new release of goose, and then checked back with this PR.

@jeremyandrews is there something that I can do in order to get this merged?

ctron avatar Mar 04 '25 12:03 ctron

Code Review - PR #602: Adding baseline to reports

This review was conducted with the assistance of an AI code analysis tool.

Critical Issues

1. Unused baseline validation in src/lib.rs (lines 1726-1731)

let _data = load_baseline_file(baseline_file).map_err(|err| GooseError::InvalidOption {
    option: "--baseline-file".to_string(),
    value: baseline_file.to_string(),
    detail: err.to_string(),
});

The baseline file is loaded but the result is discarded (_data), making the validation ineffective.

2. Bug in RequestMetric::delta_to implementation (src/report.rs lines 48-50)

self.number_of_requests.eval(other.number_of_requests);
self.number_of_requests.eval(other.number_of_requests); // DUPLICATE - should be number_of_failures
self.response_time_average.eval(other.response_time_average);

3. Documentation typo in src/config.rs (line 289) Double /// in comment.

Missing Validation

The load_baseline_file function should validate baseline compatibility:

  • Verify baseline contains metrics for compatible request paths/methods
  • Check schema version compatibility
  • Ensure consistent metric types

This validation should happen in src/metrics/common.rs load_baseline_file function.

Architecture & Performance

Delta correlation complexity: The correlate_deltas function could be optimized by sorting both arrays and using a two-pointer technique instead of creating HashMaps, eliminating allocations and improving cache performance.

Baseline loading frequency: Currently loads once per report generation. For multiple report formats, this means multiple file reads, but caching may not be critical for typical single-report usage.

Minor Issues

  • Some formatting inconsistencies
  • The error handling in baseline loading could be more consistent

Positive Notes

  • Documentation in metrics.md covers basic usage well
  • The derived serialization approach for GooseMetrics is cleaner
  • Core baseline functionality appears sound
  • Good use of type system with Value<T> enum for delta tracking

Recommendations

  1. Fix the critical bugs (unused _data, duplicate eval call, typo)
  2. Add baseline compatibility validation in load_baseline_file
  3. Consider optimizing correlate_deltas for better performance
  4. Add comprehensive tests for baseline functionality

The feature adds valuable functionality for comparing load test results over time. With the critical issues addressed, this would be a solid addition to Goose.

jeremyandrews avatar Jul 31 '25 11:07 jeremyandrews