Implementation of Tracing logger #241
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fumuran
force-pushed
the
andrew-tracing-logger
branch
from
4651c2b to
5452104
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Fumuran
force-pushed
the
andrew-tracing-logger
branch
from
512dae7 to
985e78b
Compare
|
After implementation of the tree-like logger, the output looks like so:
|
|
Could we do something closer to this? With disabled logger: With default logging level ( Also, instead of having tracing messages, we could probably incorporate extra info into the |
|
For the last point, I wonder if the output could look something like this: |
|
After the update the output looks like so:
I didn't manage to add the data, requested in this comment to the |
irakliyk
reviewed
Jan 29, 2024
Fumuran
force-pushed
the
andrew-tracing-logger
branch
from
5e15e89 to
5a00b61
Compare
Fumuran
force-pushed
the
andrew-tracing-logger
branch
from
5a00b61 to
a8b4696
Compare
irakliyk
reviewed
Jan 31, 2024
Fumuran
force-pushed
the
andrew-tracing-logger
branch
from
abca6b6 to
fa4eb3e
Compare
irakliyk
requested changes
Feb 2, 2024
examples/src/fibonacci/fib2/mod.rs
Outdated
Comment on lines
100
to
110
| let trace = info_span!( | ||
| "Generated execution trace", | ||
| registers_num = field::Empty, | ||
| steps = field::Empty | ||
| ) | ||
| .in_scope(|| { | ||
| let trace = prover.build_trace(self.sequence_length); | ||
| tracing::Span::current().record("registers_num", &format!("{}", trace.width())); | ||
| tracing::Span::current().record("steps", &format!("2^{}", trace.length().ilog2())); | ||
| trace | ||
| }); |
There was a problem hiding this comment.
For this, and all other examples:
- Let's change the message to
generate_execution_trace. - Let's change
num_registerstonum_cols. We can get this value fromTRACE_WIDTHconstant. We can also expose this constant viaFibProver.
irakliyk
requested changes
Feb 4, 2024
prover/src/lib.rs
Outdated
Comment on lines
256
to
260
| let lde_domain_size = air.lde_domain_size(); | ||
| let domain = { | ||
| let _ = info_span!("build_domain", domain_size = lde_domain_size).entered(); | ||
| StarkDomain::new(&air) | ||
| }; |
There was a problem hiding this comment.
It doesn't look like the approach I suggested works correctly (the times reported are way too small and don't add up to the final time). To make it work we'd need to do something like this:
let lde_domain_size = air.lde_domain_size();
let domain = {
let span = info_span!("build_domain", domain_size = lde_domain_size).entered();
let domain = StarkDomain::new(&air);
drop(span);
domain
};This may be fine for some other steps below (let's update them to make sure the times are captured correctly), but for this steps .in_scope() would probably be a better solution.
Also, a couple of other comments:
- Let's rename the field to
lde_domain_size. - Let's add
trace_lengthfield (this can also come fromairstruct).
|
After the recent changes logger output looks like so:
|
irakliyk
approved these changes
Feb 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR changes the logger implementation from default log to Tracing, which allows to implement informative tree-like log.