-
Notifications
You must be signed in to change notification settings - Fork 213
[PLT-87] marconi: Log progress of synced blocks #489
Conversation
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 think it still misses logging levels and tests. I also don't know why you moved Datum.hs
into the app
folder.
my bad. But is it really relevant? There's only one thing we are logging now.
What kind of tests would you like to see?
To separate marconi from the other plutus-chain-index. Otherwise dependencies of marconi get mixed up with dependencies of plutus-chain-index. |
The printed header hash prints something like It would be better to use the |
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.
Putting a from block hash in the CLI gives:
marconi: ExceptionInLinkedThread (ThreadId 12) Maybe.fromJust: Nothing
CallStack (from HasCallStack):
error, called at libraries/base/Data/Maybe.hs:148:21 in base:Data.Maybe
fromJust, called at app/Marconi.hs:142:16 in main:Main
A better error message should be printed.
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.
Tried running cabal run plutus-chain-index:exe:marconi -- --socket-path ~/.local/share/Daedalus/mainnet/cardano-node.socket --mainnet --database-path /tmp/marconi.db --slot-no 62449666 --block-hash 2777227cb7f229fe710be5e078b805b60f7d1e6f20cde0ef0c49e30f160f4a14
.
However, there's nothing stored in the database even though I've started the indexer more than 2k blocks deep.
When not setting the correct block hash and slotno in the CLI, I'm getting the error:
Maybe a prettier error message with |
@koslambrou I think I addressed all your feedback. There's no pretty instance for chain points (AFAIK) so I just implemented a function.
I am going to try the same. @raduom On my computer, next to a running node, I am doing ~500-1000 block/s does it sound ok to you? I don't want the logging to slow things down. |
I think we are fine. Good job! |
@koslambrou I cannot reproduce. My node is stuck at slot 57901784 hash 470eb682589f65aacac4c5f5e8eaa3348ebb63475d35528ec7822cc7a3dfca2f, if I start from the begingging of the same epoch it works ok:
|
Two options here: Either create an orphan instance for
Mmm... Does that mean there's no datums after slot BTW, by running the command
I would expect a |
Oh, that's weird. That would be starting exactly at the chain tip right? perhaps there's an edge case that I missed. It should say |
I used a third option. No polymorphism :)
edit: after the chat, ok I will implement |
@koslambrou when you have time I have implemented your last suggestions 🤗 |
For iohk-monitoring, reuse what we're doing in Concerning the actual output, multiple things:
Example of expected output:
|
@@ -0,0 +1,26 @@ | |||
{-# LANGUAGE FlexibleInstances #-} | |||
{-# LANGUAGE OverloadedStrings #-} | |||
{-# OPTIONS_GHC -Wno-orphans #-} |
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 think that's a good idea. We should create our own types isomorphic to Cardano.Api. The reason is because if you define an instance of Pretty
for Cardano.Api.SlotNo
, and another library you depend on does the same, you'll get an error. I've always read "Don't declare orphan instances unless you can make sure they won't come back to bite you".
@sjoerdvisscher @raduom @ak3n What are your thoughts?
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 agree in principle, but then the original simple function that @andreabedini had starts to sound quite appealing.
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 agree in principle too. I think a monomorphic function prettyChainPoint :: ChainPoint -> Doc ann
would be the best trade off because we are not really using the polymorphism anywhere (i.e. we are not passing a ChainPoint
to anything that is Pretty a => a
).
Also this is not a library but an executable, nobody can import this.
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.
We actually use the Pretty
instance with PrettyObject
to nicely integrate the LogMsg
with Pretty
, so Pretty
actually simplifies things a bit.
@andreabedini This is actually a library, not an executable. I would expect a new package like marconi-pab
which imports marconi
and indexes stuff specific for the Contract API. Also, with the query interface, someone actually needs to import marconi
if he wants to query the information that's stored in memory.
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.
following up in private
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 prefer the way it is written right now, to be honest.
I am not going to bring in effects for this.
I will do these things. |
Why not? It's not so hard to use it and bringing
You can just do: |
logInfo tracer $ | ||
renderStrict $ | ||
layoutPretty defaultLayoutOptions $ | ||
syncMsg <+> (blocksMsg $ rollbackMsg $ "Chain tip is" <+> pretty ct <> ".") |
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.
We want to know the chain tip of the synced block, not the tip of the node. So it should be pretty cp
.
Rename plutus-indexer to marconi
If the user started processing the chain at a point close to the end, the messages made it look like the synchronisation was already almost finished ("Synchronising (99.96%)"), risking confusing the user. Thats indeed the relative point on the chain that is currently being processed and has nothing to do with an ETA. This patch reworks the message to clear the meaning of that percentage. E.g. "Synchronising. Current slot 57848748 out of 57901784 (99.91%)"
Co-authored-by: koslambrou <[email protected]>
4856e8f
to
853128c
Compare
* Make the indexer use plutus-streaming * Separate Marconi executable from plutus-chain-index Rename plutus-indexer to marconi * Add logging to marconi * Fix typo * Better time formatting * Deal with malformed block hashes * Catch NoIntersectionFound and warn the user * Simplify logging logic * Improve display of chain point * Improve display of chain point [take two] * Remove duplicate dependency * Improve logging output If the user started processing the chain at a point close to the end, the messages made it look like the synchronisation was already almost finished ("Synchronising (99.96%)"), risking confusing the user. Thats indeed the relative point on the chain that is currently being processed and has nothing to do with an ETA. This patch reworks the message to clear the meaning of that percentage. E.g. "Synchronising. Current slot 57848748 out of 57901784 (99.91%)" * Use iohk-monitoring-framework * Apply suggestions from code review Co-authored-by: koslambrou <[email protected]> * Adjust logging format * Display last processed block, not chain tip * Rework imports and extensions Co-authored-by: koslambrou <[email protected]>
* Make the indexer use plutus-streaming * Separate Marconi executable from plutus-chain-index Rename plutus-indexer to marconi * Add logging to marconi * Fix typo * Better time formatting * Deal with malformed block hashes * Catch NoIntersectionFound and warn the user * Simplify logging logic * Improve display of chain point * Improve display of chain point [take two] * Remove duplicate dependency * Improve logging output If the user started processing the chain at a point close to the end, the messages made it look like the synchronisation was already almost finished ("Synchronising (99.96%)"), risking confusing the user. Thats indeed the relative point on the chain that is currently being processed and has nothing to do with an ETA. This patch reworks the message to clear the meaning of that percentage. E.g. "Synchronising. Current slot 57848748 out of 57901784 (99.91%)" * Use iohk-monitoring-framework * Apply suggestions from code review Co-authored-by: koslambrou <[email protected]> * Adjust logging format * Display last processed block, not chain tip * Rework imports and extensions Co-authored-by: koslambrou <[email protected]>
This PR implements the story PLT-87.
Example usage:
Implementation notes: