-
Notifications
You must be signed in to change notification settings - Fork 721
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
New tracing #3450
New tracing #3450
Conversation
23b4688
to
1a30c62
Compare
dfa8829
to
024922e
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.
Some initial thoughts, I made a quick pass to see if any changes would have resulted in the Plutus test failure. It doesn't seem so. I'll make another pass when the PR is actually ready for review.
e694703
to
184ecbe
Compare
3cb7c9e
to
844b9e2
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.
A few comments. I thought the consensus was to not use Condense
instances provided by ouroboros-network
and to implement our own ToJSON
instances?
New Tracing Quickstart.md
Outdated
@@ -0,0 +1,148 @@ | |||
# New Tracing Quickstart |
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.
This shouldn't be at the top level.
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.
@Jimbo4350, done
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.
The idea was to temporarily increase visibility of the Quickstart
for the new tracing system -- as we would like people to start playing with it.
But I guess it could live in doc/
as well.
b470c24
to
80f9527
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.
Mainly style fixes. Happy to green light after.
|
||
namesStartupInfo :: StartupTrace blk -> [Text] | ||
namesStartupInfo = \case | ||
StartupInfo {} -> ["StartupInfo"] |
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.
alignment
, npcByronGenesisFileHash :: !(Maybe GenesisHash) | ||
, npcByronReqNetworkMagic :: !RequiresNetworkMagic | ||
, npcByronPbftSignatureThresh :: !(Maybe Double) | ||
npcByronGenesisFile :: !GenesisFile |
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.
A perfect example of my we enforce not aligning code. It introduces unnecessary diff.
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.
I don't see what you mean
bors r+ |
Build succeeded: |
This integrates the new tracing system in
cardano-node
:trace-dispatcher
cardano-node
trace-analyzer
tx-generator
trace-forward
trace-resources
Note: I've only just opened the PR -- all the heavy work was done by @jutaro and @denisshevchenko, who get all the credit!
Note: this branch has a very squashed history, for many months of development -- for the historic branch, please see https://github.com/input-output-hk/cardano-node/tree/tracing-master-history
How to try out?
Just run
./scripts/lite/mainnet-new-tracing.sh
-- which uses this configuration: https://github.com/input-output-hk/cardano-node/tree/new-tracing/configuration/cardano/mainnet-config-new-tracing.yaml#L41-L82