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

Update --timings=json output include data similar to the HTML output #10463

Closed
wants to merge 3 commits into from
Closed

Conversation

CraZySacX
Copy link
Contributor

@CraZySacX CraZySacX commented Mar 7, 2022

Content

This PR attempts to bring the JSON output when using the --timings=json flag more inline with the data that is output when requesting HTML.

  • Added custom Serialization impls to the Timings, and Unit structs.
  • Added a report_json function to output the data in a manner similar to the HTML output.
  • Removed the output of the timing reports on stdout during unit completion when the --timings=json flag was used. That information in now collected in the final report.

Testing

This was my method. There is probably a better way.

  1. Built a release version of Cargo. cargo build --release
  2. In a test project ran cargo clean && <path-to-cargo>/target/release/cargo build --timings=json -Z unstable-options
  3. Verified that JSON output was generated in the <basedir>/target/cargo-timings/ folder.

@rust-highfive
Copy link

r? @alexcrichton

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 7, 2022
@alexcrichton
Copy link
Member

Thanks for the PR, but I'm going to be stepping down from the Cargo team so I'm going to un-assign myself from this. The Cargo team will help review this when they get a chance.

@alexcrichton alexcrichton removed their assignment Mar 8, 2022
@CraZySacX
Copy link
Contributor Author

r? @ehuss

}
.to_json_string();
crate::drop_println!(self.config, "{}", msg);
}
self.unit_times.push(unit_time);
Copy link
Contributor

Choose a reason for hiding this comment

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

There are some things that raise some flags for me

  • This moves away from serializing machine_message to internal data structures. Using code in machine_message helps raise awareness of the compatibility requirements when making changes and is more discoverable for people who want to parse the output
  • Generally we print messages as we go rather than doing one bug final message
  • The PR doesn't explain why some information was left out of the report for stablization

Overall, I'd recommend

  • Find out the thought process for what data got stablized
  • Create an issue for discussing stabilizing more information (Issues are much better for discussing requirements and design while PRs are much focusing on an implementation to resolve an Issue)
  • Focus on incrementally printing machine_message structs.

Copy link
Contributor Author

@CraZySacX CraZySacX Mar 18, 2022

Choose a reason for hiding this comment

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

I can open an issue to discuss if you'd prefer, but I have a few question:

  • The HTML reporting uses internal data structures exclusively. The intent here was to emulate the HTML reporting. Are you suggesting that all of the reporting structs move to machine_message?
  • Capturing stdout output printed on the fly in a meaningful way is a bit cumbersome. Again, emulating the HTML reporting was a goal, so the larger final output at the end was used. Should we do both?
  • Forgot to mention that the data collected was mostly inline with that the HTML report collected. It does appear I missed a couple fields. Those should definitely be included.

Copy link
Contributor

Choose a reason for hiding this comment

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

Capturing stdout output printed on the fly in a meaningful way is a bit cumbersome. Again, emulating the HTML reporting was a goal, so the larger final output at the end was used. Should we do both?

Pretty much all cargo messages (except cargo metadata) communicate like this. libtet also works this way with providing suite summary messages in addition to the individual tests.

The HTML reporting uses internal data structures exclusively. The intent here was to emulate the HTML reporting. Are you suggesting that all of the reporting structs move to machine_message?

I can definitely understand this. I love the idea of a separating out the view and model like this. At times though, shortcuts are made because not everything is locked down yet. This is why I was recommending looking into and having a discussion on why not everything is exposed.

@ehuss
Copy link
Contributor

ehuss commented Mar 23, 2022

Closing as we unfortunately don't have the capacity to accept new features of this kind at this time. Before opening a PR, please open an issue to discuss possible design choices. Just skimming this, it seems to expose internal details like Unit that I think we definitely don't want to expose at this time. I also think there are some fundamental questions about what this would be used for.

@ehuss ehuss closed this Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants