Skip to content
This repository has been archived by the owner on Apr 11, 2021. It is now read-only.

Adding logs to batch submitters #73

Merged
merged 6 commits into from
Apr 9, 2021
Merged

Adding logs to batch submitters #73

merged 6 commits into from
Apr 9, 2021

Conversation

annieke
Copy link
Contributor

@annieke annieke commented Apr 7, 2021

Description

Doing a pass of logs to make sure we have more context while debugging. Let me know if there are other key areas that need more logs!

Additional context

Improve observability!

Metadata
https://github.com/ethereum-optimism/roadmap/issues/860

@annieke annieke requested review from karlfloersch, tynes and snario April 7, 2021 00:59
@@ -166,6 +175,7 @@ export class TransactionBatchSubmitter extends BatchSubmitter {

// TODO: Remove this function and use geth for lastL1BlockNumber!
private async _updateLastL1BlockNumber() {
this.log.warn('Calling _updateLastL1BlockNumber...')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this a warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

^oo this is another spot i wanted an opinion on, i thought of putting warnings for all the functions we want to deprecate cc @karlfloersch @tynes whether we'll do that for this soon?

Copy link
Contributor

@karlfloersch karlfloersch Apr 8, 2021

Choose a reason for hiding this comment

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

@annieke shoot this function should be removed entirely — it was only used when Geth did not return an L1BlockNumber field. At this point this ( updateLastL1BlockNumber() ) is dead code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@karlfloersch it's called in _generateSequencerBatchParams, should the line that calls it be removed? bc it still seems like it's doing something

@annieke annieke requested review from snario and tynes April 7, 2021 21:54
@snario
Copy link
Contributor

snario commented Apr 8, 2021

Things I feel like are INFO level logs...

  • Beginning batch submission process (w/ chainId and batch submitter public key)
  • Querying L2 sequencer for transactions to fit in block (w/ starting block number)
  • Determined set of transactions to batch submit (w/ list of transaction IDs)
  • Submitting batch submission to chain (w/ transaction data (to, data))
  • Transaction successfully submitted to chain (w/ transaction hash)
  • Transaction successfully confirmed on-chain (w/ # confirmations)

@annieke
Copy link
Contributor Author

annieke commented Apr 8, 2021

@snario just did another pass adding those logs!

@annieke annieke requested a review from snario April 8, 2021 22:59
@@ -310,6 +349,12 @@ export class TransactionBatchSubmitter extends BatchSubmitter {
// In this case, we want to submit regardless of the batch's size.
wasBatchTruncated = true
}

this.log.info('Generated sequencer batch params', {
contexts: sequencerBatchParams.contexts,
Copy link
Contributor

Choose a reason for hiding this comment

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

How large is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the contexts is quite small (in the test with 6 transactions), the transactions are actually the longer parts of this log here. i'll keep it and de-bloat if this ends up being too large?

Comment on lines +214 to +216
this.log.info('Retrieved start block number from CTC', {
startBlock,
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be useful to log both before and after this asynchronous network call for debugging purposes

@annieke annieke merged commit 660a1fa into master Apr 9, 2021
@annieke annieke deleted the annie/better-logging branch April 9, 2021 16:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants