Skip to content

op-node/service: Deduplicate logger + Better defaults#5085

Merged
trianglesphere merged 3 commits intodevelopfrom
jg/logger
Mar 9, 2023
Merged

op-node/service: Deduplicate logger + Better defaults#5085
trianglesphere merged 3 commits intodevelopfrom
jg/logger

Conversation

@trianglesphere
Copy link
Contributor

Description

This PR does the following

  • Add logfmt as a supported log format. It just needed to be exposed to the command line.
  • Remove the op-node's log config & use op-service's version instead.
  • Change the default logger to be use logfmt instead of terminal with color
  • Change the behavior of text to mean terminal if it is a terminal or logfmt if there is no terminal.
    • The default logger is still text.
  • Added a log formatted version of the startup banner. It will still pretty-print if running in terminal mode.
    • To do this I had the move where the startup banner was logged to earlier on in the startup process.

Rationale

One of the tradeoffs that we need to make while logging is between having enough information to effectively debug problems & not providing information overload to end users or emitting too many logs which takes up disk space.

I have kept terminal logging the same for users that are directly running in a terminal, but for users that are running in docker or another non-terminal environment I switched the default to logfmt.

The logfmt format can easily be parsed by logging infrastructure while it remains more human readable than JSON. It also includes full hashes instead of the abridged terminal hash which is critical for being able to debug complex issues. One example is by abridging transaction hashes in the batcher I am not able to easily determine which transactions got included on chain and which did not. It is important that this is the default for external users because it means that when we get logs they will have enough information to be actionable. If home users are running in docker, the logs remain human readable if they need to look through them.

Log Format Example log line

t=2023-03-08T18:25:38+0000 lvl=info msg="Rollup Config"            l2_chain_id=901 l2_network="unknown L2" l1_chain_id=900 l1_network="unknown L1" l2_start_time=1,678,299,758 l2_block_hash=0x645df1804fa827dc9eae1d661501a827e128ebae1060bef7b11bb0506cf28686 l2_block_number=0 l1_block_hash=0x25348ccc26d94ae5ac0e2e2ac5c10270d4b21cbdab8fb8f937f73c507e2a08dd l1_block_number=0 regolith_time="(not configured)"

Tests

Tested manually.

@trianglesphere trianglesphere requested review from a team and protolambda as code owners March 8, 2023 19:37
@trianglesphere trianglesphere requested a review from mslipper March 8, 2023 19:37
@changeset-bot
Copy link

changeset-bot bot commented Mar 8, 2023

⚠️ No Changeset found

Latest commit: 3f0d2bd

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #5085 (4be08d5) into develop (1d5a265) will increase coverage by 0.04%.
The diff coverage is 5.12%.

❗ Current head 4be08d5 differs from pull request most recent head 3f0d2bd. Consider uploading reports for the commit 3f0d2bd to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #5085      +/-   ##
===========================================
+ Coverage    41.00%   41.04%   +0.04%     
===========================================
  Files          339      338       -1     
  Lines        20785    20756      -29     
  Branches       771      771              
===========================================
- Hits          8523     8520       -3     
+ Misses       11605    11578      -27     
- Partials       657      658       +1     
Flag Coverage Δ
bedrock-go-tests 36.71% <5.12%> (+0.04%) ⬆️
contracts-bedrock-tests 49.74% <ø> (ø)
contracts-tests 98.86% <ø> (ø)
core-utils-tests 60.41% <ø> (ø)
dtl-tests 47.15% <ø> (ø)
fault-detector-tests 33.88% <ø> (ø)
sdk-tests 38.74% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
op-node/cmd/main.go 4.87% <0.00%> (ø)
op-node/node/node.go 0.00% <ø> (ø)
op-node/rollup/types.go 81.92% <0.00%> (-9.35%) ⬇️
op-node/service.go 0.00% <ø> (ø)
op-service/log/defaults.go 0.00% <0.00%> (ø)
op-service/log/cli.go 25.00% <16.66%> (-2.78%) ⬇️
op-node/sources/batching.go 82.65% <0.00%> (-3.07%) ⬇️

Copy link
Contributor

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

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

I like the new non-terminal output - definitely helpful to have it be parsable and more detailed when not going to a terminal.

@trianglesphere trianglesphere merged commit 6b6eebe into develop Mar 9, 2023
@trianglesphere trianglesphere deleted the jg/logger branch March 9, 2023 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants