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

Standardize logging with tracing crate + implement micro-benchmarking #627

Merged
merged 14 commits into from
Aug 30, 2023

Conversation

winston-h-zhang
Copy link
Contributor

@winston-h-zhang winston-h-zhang commented Aug 21, 2023

Resolves #614.

We move away from log/env_logger/dbg! and replace everything with the unified tracing ecosystem. All the old logging infrastructure is still there, essentially with a drop in replacement of log -> tracing and tracing_subscriber. Additionally, we add the tracing_texray crate, with allows us to easily visualize spans and benchmarks of lurk. Texray can be enabled wherever and tracing offers a lot of flexibility; please see the documentation for more info (tracing, tracing-texray)

Now running

RUST_LOG=info cargo run --release --example sha256 1 f5a5fd42d16a20302798ef6ed309979b43003d2320d9f0e8ea9831a92759fb4b false

creates the following pretty output (along with the normal info level logs):

prog_start                                   305ms ├────────────────────────────────────────────────────────────────────────────────┤
  Evaluator::generate_frames                 132μs ┆
  Store::hydrate_scalar_cache                 29ms ├──────┤
  Proof::prove_recursively                   276ms         ├────────────────────────────────────────────────────────────────────────┤
    <MultiFrame as StepCircuit>::synthesize  142ms          ├────────────────────────────────────┤

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

This just needs a bit of deduplication, back to your queue @winston-h-zhang

examples/sha256_tracing.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

So this is starting to look egg-cellent! Thanks for the quick turnaround @winston-h-zhang
A couple of simpler comments inline, all around communicating what's going on, and which I think are needed to complete the "wow" effect of this PR, and then I think we can merge.

src/main.rs Outdated Show resolved Hide resolved
clutch/src/main.rs Outdated Show resolved Hide resolved
@@ -153,7 +155,12 @@ enum Sha256Coproc<F: LurkField> {
/// Run the example in this file with
/// `cargo run --release --example sha256 1 f5a5fd42d16a20302798ef6ed309979b43003d2320d9f0e8ea9831a92759fb4b false`
Copy link
Contributor

@huitseeker huitseeker Aug 29, 2023

Choose a reason for hiding this comment

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

Please annotate with the command that would allow printing the TexRay layer to stdout, and recall that command + the output in the PR summary for reviewers to easily get the point of the PR.

fcomm/src/bin/fcomm.rs Outdated Show resolved Hide resolved
src/proof/nova.rs Show resolved Hide resolved
src/proof/nova.rs Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@huitseeker huitseeker changed the title Standardize logging with tracing crate Standardize logging with tracing crate + implement micro-benchmarking Aug 29, 2023
winston-h-zhang and others added 13 commits August 29, 2023 12:40
- Removed `log` as a workspace dependency and replaced it with `tracing` throughout the project,
- Updated clippy linting configuration in `.cargo/config`.
- Adapted from `log` crate's `info` module to `tracing` crate's `info` module in `lurk-metrics/src/data.rs`
Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

Tip top, thank you so much @winston-h-zhang !

@huitseeker huitseeker enabled auto-merge August 29, 2023 22:31
@huitseeker huitseeker added this pull request to the merge queue Aug 30, 2023
Merged via the queue into master with commit dd91a68 Aug 30, 2023
6 checks passed
@huitseeker huitseeker deleted the hz/tracing branch August 30, 2023 01:36
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.

Rationalize "events" (logs) coming out of the code base
4 participants