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

Improved logging #3001

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

Improved logging #3001

wants to merge 3 commits into from

Conversation

pedromiguelmiranda
Copy link
Contributor

@pedromiguelmiranda pedromiguelmiranda commented Jan 14, 2025

Improved eth1 logging:

  • using nimbus-eth2 options
  • log file support removal
  • auto detection of tty, colour support and related.

fix #2544

@tersec
Copy link
Contributor

tersec commented Jan 18, 2025

Yeah, this is arguably a nim-json-serialization bug:

/home/runner/work/nimbus-eth1/nimbus-eth1/nimbus/common/common.nim(128, 5) template/generic instantiation of `info` from here
/home/runner/work/nimbus-eth1/nimbus-eth1/vendor/nim-chronicles/chronicles.nim(380, 10) template/generic instantiation of `log` from here
/home/runner/work/nimbus-eth1/nimbus-eth1/nimbus/common/common.nim(134, 7) template/generic instantiation of `expandItIMPL` from here
/home/runner/work/nimbus-eth1/nimbus-eth1/vendor/nim-chronicles/chronicles/log_output.nim(835, 39) template/generic instantiation of `setProperty` from here
/home/runner/work/nimbus-eth1/nimbus-eth1/vendor/nim-chronicles/chronicles/log_output.nim(772, 10) template/generic instantiation of `[]=` from here
/home/runner/work/nimbus-eth1/nimbus-eth1/vendor/nim-chronicles/chronicles/log_output.nim(745, 17) template/generic instantiation of `writeField` from here
/home/runner/work/nimbus-eth1/nimbus-eth1/vendor/nim-json-serialization/json_serialization/writer.nim(94, 8) template/generic instantiation of `writeValue` from here
/home/runner/work/nimbus-eth1/nimbus-eth1/vendor/nim-json-serialization/json_serialization/writer.nim(387, 23) template/generic instantiation of `writeRecordValue` from here
/home/runner/work/nimbus-eth1/nimbus-eth1/vendor/nim-json-serialization/json_serialization/writer.nim(203, 8) template/generic instantiation of `enumInstanceSerializedFields` from here
/home/runner/work/nimbus-eth1/nimbus-eth1/vendor/nim-serialization/serialization/object_serialization.nim(48, 40) Error: type mismatch
Expression: fieldPairs(value)
  [1] value: uint64

Expected one of (first mismatch at [position]):
[1] iterator fieldPairs[S: tuple | object; T: tuple | object](x: S; y: T): tuple[
    key: string, a, b: RootObj]
[1] iterator fieldPairs[T: tuple | object](x: T): tuple[key: string, val: RootObj]

is because nim-json-serialization's writeValue only checks for SomeInteger, before fairly shortly assuming it has to be an object of some type, a heuristic which fails in the case of a distinct of SomeInteger:

  elif value is SomeInteger:      # this case gets skipped
    w.stream.writeText value

  elif value is SomeFloat:
    # TODO Implement writeText for floats
    #      to avoid the allocation here:
    append $value

  elif value is (seq or array or openArray) or
      (value is distinct and distinctBase(value) is (seq or array or openArray)):
    when value is distinct:
      w.writeArray(distinctBase value)
    else:
      w.writeArray(value)

  elif value is (distinct or object or tuple):
    mixin flavorUsesAutomaticObjectSerialization

    type Flavor = JsonWriter.Flavor
    const isAutomatic =
      flavorUsesAutomaticObjectSerialization(Flavor)

    when not isAutomatic:
      const typeName = typetraits.name(type value)
      {.error: "Please override writeValue for the " & typeName & " type (or import the module where the override is provided)".}

    when value is distinct:
      # it tries to do this, but this is inappropriate for a `distinct SomeInteger`
      writeRecordValue(w, distinctBase(value, recursive = false))

This only trigers when flavorUsesAutomaticObjectSerialization(Flavor), otherwise it will (more or less defensibly reasonable) report that it can't, in this case, serialize EthTime.

But when automatic serialization is enabled, this at least some kind of n-j-s issue.

@pedromiguelmiranda
Copy link
Contributor Author

Initially, I also agreed that it was a Nim-Json-Serializer bug. After some talks, I've changed my mind.
njs is a public and generic library, and so any used defined types should be implemented by the user using the public available handlers (documented on njs README file).

@tersec
Copy link
Contributor

tersec commented Jan 23, 2025

Initially, I also agreed that it was a Nim-Json-Serializer bug. After some talks, I've changed my mind. njs is a public and generic library, and so any used defined types should be implemented by the user using the public available handlers (documented on njs README file).

Yeah, the bug I had in mind was mainly that it even tried to use the fieldPairs codepath with a non-object.

It's fine to group elif value is (distinct or object or tuple): but then it doesn't make sense to assume that it must specifically be an object.

Pedro Miranda added 3 commits January 24, 2025 16:23
- log file support removal
- auto detection of tty, colour support and related.
 for some eth, web3 and confutils types
@pedromiguelmiranda pedromiguelmiranda changed the title Improved logging[Wip] Improved logging Jan 24, 2025
@pedromiguelmiranda pedromiguelmiranda marked this pull request as ready for review January 24, 2025 17:37
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.

Implement logging options from eth2
3 participants