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

Refactor reports/metrics, add JSON and markdown report #600

Merged
merged 9 commits into from
Aug 27, 2024

Conversation

ctron
Copy link
Contributor

@ctron ctron commented Jul 29, 2024

This PR refactors the code of the metrics and report module a bit. It extracts the common part into a common module, allowing to prepare the data consistently and then rendering it out into HTML, and also serializing the data into JSON.

For that the --report-file argument has been changed into a Vec<String> to allow providing more than one file. It uses the file extension as an indicator of which file type should be created.

In addition, this should make it easier to implement a markdown report too.

@ctron ctron mentioned this pull request Jul 29, 2024
@ctron
Copy link
Contributor Author

ctron commented Jul 30, 2024

I updated the PR to make clippy happy.

Although these are not files touched by the PR, it looks like newer Rust versions brought in new lints that now fail.

@ctron ctron changed the title Refactor reports/metrics, add JSON report Refactor reports/metrics, add JSON and markdown report Jul 30, 2024
@ctron
Copy link
Contributor Author

ctron commented Jul 30, 2024

I updated the PR to support markdown as well. It should be pretty close to the HTML report, but without the charts.

@jeremyandrews
Copy link
Member

This looks great from a read through on my phone. I’m traveling internationally but will find time to test it and get it merged.

In a follow up PR, it would be great to update the book documentation too.

@ctron
Copy link
Contributor Author

ctron commented Jul 30, 2024

This looks great from a read through on my phone. I’m traveling internationally but will find time to test it and get it merged.

In a follow up PR, it would be great to update the book documentation too.

Cool. Take your time. I can update the PR over time. I am currently also trying to add the baseline feature. We can split that off, but it's based on the other changes anyway.

@JimFuller-RedHat
Copy link

does this mean a new release as well ? nudge nudge

@ctron
Copy link
Contributor Author

ctron commented Jul 31, 2024

I pushed the change for baseline comparison too. Of course we can split this off. Looks like this:

image

@jeremyandrews
Copy link
Member

I pushed the change for baseline comparison too. Of course we can split this off.

Please split this off -- I'd like to get the first bits committed first.

@ctron ctron force-pushed the feature/reports_2 branch 2 times, most recently from 38669ea to 26ed40c Compare August 1, 2024 07:39
@ctron
Copy link
Contributor Author

ctron commented Aug 1, 2024

I pushed the change for baseline comparison too. Of course we can split this off.

Please split this off -- I'd like to get the first bits committed first.

I split it off (#602) and rebased the PR.

@jeremyandrews
Copy link
Member

I'm finally starting to test this now. We need to improve the documentation, as it's non-obvious what format of report files are supported. Further, it feels like there should be some sort of validation? For example, if you accidentally type --report-file report.htm it will create the file but not log anything there. While this isn't the expected filename on a Unix-based system, wouldn't it be legit on Windows? And, if you type --report-file report.foo it will happily create that filename, but then it won't write data to it.

Copy link
Member

@jeremyandrews jeremyandrews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is fantastic, thanks for the contribution! However, there are a few small issues to resolve, see the earlier comments. In addition, it would be very helpful to:

Cargo.toml Show resolved Hide resolved
src/metrics.rs Show resolved Hide resolved
src/metrics.rs Outdated Show resolved Hide resolved
src/metrics.rs Show resolved Hide resolved
src/report/markdown.rs Outdated Show resolved Hide resolved
@ctron
Copy link
Contributor Author

ctron commented Aug 26, 2024

Updated the PR, and the docs as well.

Cargo.toml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@jeremyandrews jeremyandrews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks for all your updates! I'm only confused about the test failures: locally I can't duplicate this, and these tests that are failing should be ignored. Any ideas?

@jeremyandrews
Copy link
Member

This looks great, thanks for all your updates! I'm only confused about the test failures: locally I can't duplicate this, and these tests that are failing should be ignored. Any ideas?

I see, we're running with --all-features, so adding back the gaggle feature is causing these tests to run, while the contained code is no longer supported (for now).

@jeremyandrews
Copy link
Member

We can easily fix the gaggle issue in a follow-up, I've confirmed all other tests run correctly and without errors.

This new functionality is greatly appreciated, thanks for the contribution!

@jeremyandrews jeremyandrews merged commit 2f9f6a5 into tag1consulting:main Aug 27, 2024
1 of 2 checks passed
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