Skip to content
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

Debug logger namespace standardization #3692

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

scorbajio
Copy link
Contributor

@scorbajio scorbajio commented Sep 20, 2024

This change addresses the issues raised in #3689.

@scorbajio scorbajio force-pushed the debug-namespace-standardization branch from 215484a to 7b23b79 Compare September 20, 2024 20:04
@scorbajio
Copy link
Contributor Author

The logger namespaces are now using lowercase tags.

trie:

  trie:find_path || Stack: ExtensionNode, BranchNode, BranchNode, BranchNode, LeafNode +0ms
  trie Setting root to 0x96ce81d93272f41719326f5cd4333c88f7ad403cc4404efc0fa62ffb10a88035 +2ms
  trie:put Key: 0x0c222c +0ms
  trie:put Value: 0x0c222c +0ms
  trie:find_path Target (6): [0,12,2,2,2,12] +0ms
  trie:find_path Walking trie from ROOT: 0x96ce81d93272f41719326f5cd4333c88f7ad403cc4404efc0fa62ffb10a88035 +0ms
  trie:lookup_node:by_hash 0x96ce81d93272f41719326f5cd4333c88f7ad403cc4404efc0fa62ffb10a88035 +0ms
  trie:lookup_node:by_hash ExtensionNode found in DB +0ms
  trie:find_path:extension_node Comparing node key to expected
  trie:find_path:extension_node || Node_Key: [0]
  trie:find_path:extension_node || Expected: [0]
  trie:find_path:extension_node || Matching: [true]
  trie:find_path:extension_node              +0ms
  trie:find_path:extension_node NextNode: 138,73,186,20,217,38,102,24,19,23,119,204,15,235,225,169,246,72,131,186,184,142,11,21,205,144,236,100,68,104,239,255 +0ms
  trie:lookup_node:by_hash 0x8a49ba14d9266618131777cc0febe1a9f64883bab88e0b15cd90ec644468efff +0ms
  trie:lookup_node:by_hash BranchNode found in DB +0ms
  trie:find_path:branch_node Looking for node on branch index: [12] +0ms
  trie:find_path:branch_node:12 NodeHash: 0xf2a8cf9296f999d637c17e03303af4aa4f0e5628d54d0e7ec3b6642015232f25 +0ms
  trie:lookup_node:by_hash 0xf2a8cf9296f999d637c17e03303af4aa4f0e5628d54d0e7ec3b6642015232f25 +0ms
  trie:lookup_node:by_hash BranchNode found in DB +0ms
  trie:find_path:branch_node Looking for node on branch index: [2] +0ms
  trie:find_path:branch_node:2 NodeHash: 0xcee1ccd3c512df214f62cf72006a64b394f6ffd73cfa55ef35e52711f6ef3b83 +0ms
  trie:lookup_node:by_hash 0xcee1ccd3c512df214f62cf72006a64b394f6ffd73cfa55ef35e52711f6ef3b83 +0ms
  trie:lookup_node:by_hash BranchNode found in DB +0ms
  trie:find_path:branch_node Looking for node on branch index: [2] +0ms
  trie:find_path:branch_node:2 Raw_Node: 32,44,99,114,101,97,116,101,32,116,104,101,32,108,97,115,116,32,98,114,97,110,99,104 +0ms
  trie:lookup_node:raw_node LeafNode +0ms

verkle:

  verkle:get Suffix: 1; Value: 0x320122e8584be00d000000000000000000000000000000000000000000000000 +0ms
  verkle:get Stem: 0x318dfa512b6f3237a2d4763cf49bf26de3b617fb0cabe38a97807a5549df4d; Suffix: 2 +0ms
  verkle:find_path Path (31): [0x318dfa512b6f3237a2d4763cf49bf26de3b617fb0cabe38a97807a5549df4d] +0ms
  verkle:find_path Starting with Root Node: [0x120744309fa260e19b58b2f5da88073fc62f883cd9cb9b0103d37d4002199802] +0ms
  verkle:find_path Partial Path 0x318d - found next node in path 0x41d4cf67efa1b8b71c7ae8345666433bf06ad7b9335642fc129122887a259e02. +0ms
  verkle:find_path Partial Path 0x318dfa51 - found next node in path 0x8225a4e1f63a9ed69ba4a486d5ce0d741cbb881054160d3b2f1f86941cab3406. +0ms

Copy link

codecov bot commented Sep 20, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.07%. Comparing base (4470cc3) to head (a55b191).
Report is 74 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block ?
blockchain 87.88% <83.33%> (?)
client ?
common 91.47% <ø> (?)
tx ?
wallet ?

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

Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

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

Some small nits

const canonicalHeadHash = (await this.getCanonicalHeadHeader()).hash()
let canonicalHeadHash: Uint8Array | undefined
if (this.DEBUG) {
canonicalHeadHash = (await this.getCanonicalHeadHeader()).hash()
Copy link
Member

Choose a reason for hiding this comment

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

Ah nice find that this is only necessary when in debug mode 😄

@@ -352,7 +355,7 @@ export class Blockchain implements BlockchainInterface {

this.DEBUG &&
this._debug(
`Canonical head set from ${bytesToHex(canonicalHeadHash)} to ${bytesToHex(hash!)} (number ${bigIntToHex(canonicalHead)})`,
`Canonical head set from ${canonicalHeadHash ? bytesToHex(canonicalHeadHash) : 'unknown'} to ${bytesToHex(hash!)} (number ${bigIntToHex(canonicalHead)})`,
Copy link
Member

Choose a reason for hiding this comment

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

Since this.DEBUG is true in this case, we know canonicalHeadHash is set, so the ? : logic should be deleted here (if TypeScript complains, we know canonicalHeadHash is set so you can make TS know it is set by using canonicalHeadHash! (add exclamation mark))

| `verkle:*` | verbose debug logging for all verkle methods |
| Logger | Description |
| ------------------- | ------------------------------------ |
| `verkle:put` | a verkle put operation has occurred |
Copy link
Member

Choose a reason for hiding this comment

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

There should be an addition for verkle:# here.

@@ -466,7 +466,7 @@ The following options are available:

| Logger | Description |
| ------------------- | ------------------------------------------------ |
| `client:fetcher` | This option enables logging for all fetchers |
| `client:fetcher:#` | This option enables logging for all fetchers |
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any changes in the client code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make debug loggers throughout monorepo uniform in namespacing
2 participants