-
Notifications
You must be signed in to change notification settings - Fork 89
feat(consensus): add tracing instrumentation and logging to consensus #2138
Conversation
99ad6c4
to
41c67ba
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2138 +/- ##
==========================================
+ Coverage 65.67% 65.90% +0.23%
==========================================
Files 135 135
Lines 17886 17808 -78
Branches 17886 17808 -78
==========================================
- Hits 11747 11737 -10
+ Misses 4852 4784 -68
Partials 1287 1287 ☔ View full report in Codecov by Sentry. |
77789e7
to
688b677
Compare
a552685
to
5020790
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @matan-starkware)
crates/papyrus_node/src/main.rs
line 101 at r1 (raw file):
) -> anyhow::Result<JoinHandle<Result<(), ConsensusError>>> { let Ok(validator_id) = env::var("CONSENSUS_VALIDATOR_ID") else { debug!("CONSENSUS_VALIDATOR_ID is not set. Not running consensus.");
Can remain info IMO
Code quote:
debug!
crates/sequencing/papyrus_consensus/src/lib.rs
line 23 at r1 (raw file):
// TODO(dvir): add test for this. #[instrument(
Consider adding explicit level
Code quote:
#[instrument(
crates/sequencing/papyrus_consensus/src/lib.rs
line 24 at r1 (raw file):
// TODO(dvir): add test for this. #[instrument( skip(context, start_height, network_receiver),
Why skipping start height?
Code quote:
start_height
crates/sequencing/papyrus_consensus/src/single_height_consensus.rs
line 40 at r1 (raw file):
} #[instrument(skip(self), fields(height=self.height.0), level = "info")]
Why not debug?
Code quote:
level = "info"
crates/sequencing/papyrus_consensus/src/single_height_consensus.rs
line 76 at r1 (raw file):
skip(self, init, content_receiver, fin_receiver), fields(height = %self.height), level = "info"
Why not debug?
Code quote:
level = "info"
5020790
to
9ee7c83
Compare
688b677
to
394f4c5
Compare
3d79e30
to
1bdc597
Compare
The base branch was changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @dan-starkware)
crates/sequencing/papyrus_consensus/src/lib.rs
line 24 at r1 (raw file):
Previously, dan-starkware wrote…
Why skipping start height?
Because these fields are printed out with every single log within this scope. I only want to print the starting height once when I begin.
1bdc597
to
89f5ad9
Compare
89f5ad9
to
aff6ab1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @matan-starkware)
crates/sequencing/papyrus_consensus/src/lib.rs
line 24 at r1 (raw file):
Previously, matan-starkware wrote…
Because these fields are printed out with every single log within this scope. I only want to print the starting height once when I begin.
That's not necessarily a bad thing, I'm fine either way
This change is