Skip to content
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

Adding baseline to reports #602

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ctron
Copy link
Contributor

@ctron ctron commented Aug 1, 2024

Based on #600

@jeremyandrews
Copy link
Member

jeremyandrews commented Aug 28, 2024

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))

@ctron ctron marked this pull request as ready for review August 28, 2024 13:03
@ctron
Copy link
Contributor Author

ctron commented Aug 28, 2024

This new feature would benefit from some documentation.

Yes, I can understand that 🤣 … 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
Copy link
Contributor Author

ctron commented Aug 28, 2024

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
Copy link
Contributor Author

ctron commented Aug 28, 2024

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
Copy link
Contributor Author

ctron commented Aug 28, 2024

And here is the corresponding issue: serde-rs/json#202 … It might make sense to follow the way of adding a custom deserializer: serde-rs/json#202 (comment)

Serde serializes f32s of value NaN to `null`, but doesn't serialize them
back from `null` to NaN. For that we need a custom deserialize.

Also see: <serde-rs/json#202>
@ctron
Copy link
Contributor Author

ctron commented Aug 28, 2024

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

@jeremyandrews
Copy link
Member

jeremyandrews commented Aug 28, 2024

 % 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

@ctron
Copy link
Contributor Author

ctron commented Aug 28, 2024

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

@ctron
Copy link
Contributor Author

ctron commented Aug 28, 2024

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

Done.

@JimFuller-RedHat
Copy link

LGTM (and yes I would say that!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants