feat: create logging proxy #663

emizzle · 2023-12-20T21:49:54Z

Add a logging proxy to achieve several goals:

Fix json logging output (run with --log-format=json) which was effectively broken for many types.
Ability to specify log output for textlines and json sinks together or separately
- This is useful if consuming json in some kind of log parser and need valid json with real values
- eg a shortened Cid is nice to see in a text log in stdout, but won't provide a real Cid when parsed in json
Remove usages of nim-json-serialization from the codebase
Remove need to declare writeValue for new types
Remove need to avoid importing or exporting toJson, %, %* to prevent conflicts

When declaring a new type, one should consider importing the codex/logutils module, and specifying formatIt. If textlines log output and json log output need to be different, overload formatIt appropriately. If a json serialization is needed, it can be declared with a % proc. Only codex/logutils needs to be imported.

With this PR in:

Instead of importing pkg/chronicles, import pkg/codex/logutils
- most of chronicles is exported by logutils
Instead of importing std/json, import pkg/codex/utils/json
- std/json is exported by utils/json which is exported by logutils
Instead of importing pkg/nim-json-serialization, import pkg/codex/utils/json
- one of the goals is to remove the use of nim-json-serialization

import pkg/codex/logutils

type
  BlockAddress* = object
    case leaf*: bool
    of true:
      treeCid* {.serialize.}: Cid
      index* {.serialize.}: Natural
    else:
      cid* {.serialize.}: Cid

logutils.formatIt(LogFormat.textLines, BlockAddress):
  if it.leaf:
    "treeCid: " & shortLog($it.treeCid) & ", index: " & $it.index
  else:
    "cid: " & shortLog($it.cid)

logutils.formatIt(LogFormat.json, BlockAddress): %it

# chronicles textlines output
TRC test                                       tid=14397405 ba="treeCid: zb2*fndjU1, index: 0"
# chronicles json output
{"lvl":"TRC","msg":"test","tid":14397405,"ba":{"treeCid":"zb2rhgsDE16rLtbwTFeNKbdSobtKiWdjJPvKEuPgrQAfndjU1","index":0}}

In this case, BlockAddress is just an object, so utils/json can handle serializing it without issue (only fields annotated with {.serialize.} will serialize (opt-in).

If one so wished, another option for the textlines log output, would be to simply toString the serialised json:

logutils.formatIt(LogFormat.textLines, BlockAddress): $ %it
# or, more succinctly:
logutils.formatIt(LogFormat.textLines, BlockAddress): it.toJson

In that case, both the textlines and json sinks would have the same output, so we could reduce this even further by not specifying a LogFormat:

type
  BlockAddress* = object
    case leaf*: bool
    of true:
      treeCid* {.serialize.}: Cid
      index* {.serialize.}: Natural
    else:
      cid* {.serialize.}: Cid

logutils.formatIt(BlockAddress): %it

# chronicles textlines output
TRC test                                       tid=14400673 ba="{\"treeCid\":\"zb2rhgsDE16rLtbwTFeNKbdSobtKiWdjJPvKEuPgrQAfndjU1\",\"index\":0}"
# chronicles json output
{"lvl":"TRC","msg":"test","tid":14400673,"ba":{"treeCid":"zb2rhgsDE16rLtbwTFeNKbdSobtKiWdjJPvKEuPgrQAfndjU1","index":0}}

Note

Special thanks to @elcritch for the help on this one!

The logging proxy: - prevents the need to import chronicles (as well as export except toJson), - prevents the need to override `writeValue` or use or import nim-json-seralization elsewhere in the codebase, allowing for sole use of utils/json for de/serialization, - and handles json formatting correctly in chronicles json sinks

Not specifying a LogFormat will apply the formatting to both textlines and json sinks. Specifying a LogFormat will apply the formatting to only that sink.

We only need to import utils/json instead of std/json

Was causing unit tests to fail on Windows.

Windows was erroring with `could not load: pcre64.dll`. Instead of fixing that error, remove the pcre usage :)

Both json and textlines formatIt were not needed, and could be combined into one formatIt

debug output and logformat of json for integration test logs

Before the changes in this branch, fromBytes was likely being resolved by nim-stew, or other dependency. With the changes in this branch, that dependency was removed and fromBytes could no longer be resolved. By exporting fromBytes from nim-poseidon, the correct resolution is now happening.

emizzle · 2024-01-19T05:02:08Z

codex/logutils.nim

+        json.`%`(body)
+      else:
+        newJNull()
+


Oddly enough, if I add:

import questionable/results proc setProperty*(r: var JsonRecord, key: string, opt: ?!T) = # ...

The compiler complains that it cannot resolve the symbol ?!. The same happens if I use the long hand Result[T, ref CatchableError] as well. nimsuggest offers a further explanation that this may be due to a circular depdency, BUT HOW???

undeclared identifier: '?!' candidates (edit distance, scope distance); see '--spellSuggest': (1, 3): '!' [template declared in /Users/egonat/repos/status-im/nim-codex/vendor/questionable/questionable/options.nim(23, 10)] (1, 3): '?' [template declared in /Users/egonat/repos/status-im/nim-codex/vendor/questionable/questionable/options.nim(17, 10)] This might be caused by a recursive module dependency: /Users/egonat/repos/status-im/nim-codex/codex/logutils.nim imports /Users/egonat/repos/status-im/nim-codex/codex/logutils.nim /Users/egonat/repos/status-im/nim-codex/codex/logutils.nim imports /Users/egonat/repos/status-im/nim-codex/codex/codex.nim /Users/egonat/repos/status-im/nim-codex/codex/codex.nim imports /Users/egonat/repos/status-im/nim-codex/codex/logutils.nim /Users/egonat/repos/status-im/nim-codex/codex/logutils.nim imports /Users/egonat/repos/status-im/nim-codex/codex/codex.nim /Users/egonat/repos/status-im/nim-codex/codex/codex.nim imports /Users/egonat/repos/status-im/nim-codex/codex/codex.nim /Users/egonat/repos/status-im/nim-codex/codex/codex.nim imports /Users/egonat/repos/status-im/nim-codex/codex/logutils.nim /Users/egonat/repos/status-im/nim-codex/codex/logutils.nim imports /Users/egonat/repos/status-im/nim-codex/codex/node.nim /Users/egonat/repos/status-im/nim-codex/codex/node.nim imports /Users/egonat/repos/status-im/nim-codex/codex/node.nim /Users/egonat/repos/status-im/nim-codex/codex/node.nim imports /Users/egonat/repos/status-im/nim-codex/codex/node.nim /Users/egonat/repos/status-im/nim-codex/codex/node.nim imports /Users/egonat/repos/status-im/nim-codex/codex/node.nim /Users/egonat/repos/status-im/nim-codex/codex/node.nim imports /Users/egonat/repos/status-im/nim-codex/codex/node.nim /Users/egonat/repos/status-im/nim-codex/codex/node.nim imports /Users/egonat/repos/status-im/nim-codex/codex/node.nim /Users/egonat/repos/status-im/nim-codex/codex/node.nim imports /Users/egonat/repos/status-im/nim-codex/codex/codex.nim /Users/egonat/repos/status-im/nim-codex/codex/codex.nim imports /Users/egonat/repos/status-im/nim-codex/codex/logutils.nim /Users/egonat/repos/status-im/nim-codex/codex/logutils.nim imports /Users/egonat/repos/status-im/nim-codex/codex/logutils.nim /Users/egonat/repos/status-im/nim-codex/codex/logutils.nim imports /Users/egonat/repos/status-im/nim-codex/codex/codex.nim /Users/egonat/repos/status-im/nim-codex/codex/codex.nim imports /Users/egonat/repos/status-im/nim-codex/codex/logutils.nim /Users/egonat/repos/status-im/nim-codex/codex/logutils.nim imports /Users/egonat/repos/status-im/nim-codex/codex/codex.nim /Users/egonat/repos/status-im/nim-codex/codex/codex.nim imports /Users/egonat/repos/status-im/nim-codex/codex/logutils.nim /Users/egonat/repos/status-im/nim-codex/codex/logutils.nim imports /Users/egonat/repos/status-im/nim-codex/codex/logutils.nim /Users/egonat/repos/status-im/nim-codex/codex/logutils.nim imports /Users/egonat/repos/status-im/nim-codex/codex/logutils.nim /Users/egonat/repos/status-im/nim-codex/codex/logutils.nim imports /Users/egonat/repos/status-im/nim-codex/codex/codex.nim /Users/egonat/repos/status-im/nim-codex/codex/codex.nim imports /Users/egonat/repos/status-im/nim-codex/codex/node.nim /Users/egonat/repos/status-im/nim-codex/codex/node.nim imports /Users/egonat/repos/status-im/nim-codex/codex/chunker.nim /Users/egonat/repos/status-im/nim-codex/codex/chunker.nim imports /Users/egonat/repos/status-im/nim-codex/codex/blocktype.nim /Users/egonat/repos/status-im/nim-codex/codex/blocktype.nim imports /Users/egonat/repos/status-im/nim-codex/codex/codextypes.nim /Users/egonat/repos/status-im/nim-codex/codex/codextypes.nim imports /Users/egonat/repos/status-im/nim-codex/codex/units.nim /Users/egonat/repos/status-im/nim-codex/codex/units.nim imports /Users/egonat/repos/status-im/nim-codex/codex/logutils.nim

logutils does not import codex which seems to be the root of the circular dependency.

this looks bad overall, have we figured out the root cause?

By removing the {.dirty.} annotation and injecting the needed it vars, the issue was resolved. questionable/results is also now exported, so its possible this was the cause of the undeclared identifier.

dryajov

LGTM

emizzle mentioned this pull request Jan 9, 2024

exports fromBytes from the library codex-storage/nim-poseidon2#19

Merged

emizzle force-pushed the feat/logging-proxy branch 3 times, most recently from 27418b8 to b8d265c Compare January 9, 2024 04:55

benbierens approved these changes Jan 18, 2024

View reviewed changes

emizzle added 15 commits January 19, 2024 12:00

Rename logging -> logutils to avoid ambiguity with common names

a903415

clean up

c18b338

add setProperty for JsonRecord, remove nim-json-serialization conflict

1a11dbe

Allow specifying textlines and json format separately

492a429

Not specifying a LogFormat will apply the formatting to both textlines and json sinks. Specifying a LogFormat will apply the formatting to only that sink.

remove unneeded usages of std/json

99302a3

We only need to import utils/json instead of std/json

move serialization from rest/json to utils/json so it can be shared

ba66709

fix NoColors ambiguity

c9d7b1a

Was causing unit tests to fail on Windows.

Remove nre usage to fix Windows error

07c14e1

Windows was erroring with `could not load: pcre64.dll`. Instead of fixing that error, remove the pcre usage :)

Add logutils module doc

ad0f3e9

Shorten logutils.formatIt for NBytes

d658a02

Both json and textlines formatIt were not needed, and could be combined into one formatIt

remove debug integration test config

5299d4e

debug output and logformat of json for integration test logs

Use ## module doc to support docgen

865b879

fixes to get compiling after rebasing master

ca98d10

emizzle force-pushed the feat/logging-proxy branch from b8d265c to ca98d10 Compare January 19, 2024 04:57

emizzle commented Jan 19, 2024

View reviewed changes

Add support for Result types being logged using formatIt

09b7373

dryajov approved these changes Jan 23, 2024

View reviewed changes

dryajov merged commit de88fd2 into master Jan 23, 2024
8 checks passed

dryajov deleted the feat/logging-proxy branch January 23, 2024 07:35

dryajov added a commit that referenced this pull request Feb 16, 2024

merge #663

3e687a9

dryajov added a commit that referenced this pull request Feb 19, 2024

merge #663

661e705

dryajov added a commit that referenced this pull request Feb 19, 2024

merge #663

189ba80

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: create logging proxy #663

feat: create logging proxy #663

emizzle commented Dec 20, 2023

emizzle Jan 19, 2024

dryajov Jan 22, 2024

emizzle Jan 23, 2024

dryajov left a comment

feat: create logging proxy #663

feat: create logging proxy #663

Conversation

emizzle commented Dec 20, 2023

Note

emizzle Jan 19, 2024

Choose a reason for hiding this comment

dryajov Jan 22, 2024

Choose a reason for hiding this comment

emizzle Jan 23, 2024

Choose a reason for hiding this comment

dryajov left a comment

Choose a reason for hiding this comment