status-im / nim-chronicles

A crafty implementation of structured logging for Nim.
Apache License 2.0
154 stars 20 forks source link

Doesn't support Port logging in json mode #98

Open Menduist opened 3 years ago

Menduist commented 3 years ago

eg: https://github.com/status-im/nim-libp2p/pull/593/checks?check_run_id=2905069466

Useful to dump addresses, for instance

arnetheduck commented 3 years ago

Somewhat unintuitively, to make this work you need to import https://github.com/status-im/nim-json-serialization/blob/master/json_serialization/std/net.nim

Menduist commented 3 years ago

So exporting it inside chronicles solves the issue. Or is it better to export it in json_serialization directly?

arnetheduck commented 3 years ago

well, that's a bit of an open question, but generally, neither chronicles or json_ser - rather, the module that uses Port and chronicles should import said module - if it also exports something public with Port, it should export net as well.

The rationale here is that chronicles and json_ser should not cause dependencies on all std library modules (ie Port is just one example, but there's Table, etc)

arnetheduck commented 3 years ago

https://status-im.github.io/nim-style-guide/language.import.html

Menduist commented 3 years ago

Ok make sense, then it could be interesting to make a list of type -> module required to log it somewhere in the docs

arnetheduck commented 3 years ago

well, the std ones are gathered where they are, otherwise we tend to put json serializers in the same module as the type definition, for types we control and know will be logged.

The difficulty lies in discerning logging from other serializations - ie the json serialization format depends not only on the type but also on the specific target (log output vs conforming to a spec - for example an integer might need to be serialized as hex or decimal for different json output targets) cc @zah

arnetheduck commented 3 years ago

sometimes, there exists a "canonical" serialization but often not - Nim tends to err on the side of overly generic functions that make spotting the lack of a specific serializer difficult - specially in cases where serializers are declared in different modules and you end up with different serializations depending on your (transitive) import list - this is a significant issue and a bit of a design flaw in many of the generic serializers

zah commented 3 years ago

The difficulty lies in discerning logging from other serializations - ie the json serialization format depends not only on the type but also on the specific target (log output vs conforming to a spec - for example an integer might need to be serialized as hex or decimal for different json output targets) cc @zah

This is already addressed by the JSON flavors feature in nim-json-serialization. What we can do now to give the user more control would be to define a separate flavor for Chronicles.

Otherwise, I've proposed some ways to solve this "missing import" problem at the language level, but I don't expect to see progress on this front: https://github.com/nim-lang/RFCs/issues/380#issuecomment-857569264

Another possible feature is to allow the creation of flavors that don't use the default serialization behavior and always require an explicit overload to be in scope, but unfortunately this won't be compatible with some of the more advanced features of nim-serialization such as using custom serialization logic for a specific field:

https://github.com/status-im/nimbus-eth2/blob/44f652f704f14c67e648f2b14dee652d44b858c5/beacon_chain/spec/datatypes/phase0.nim#L262-L275

This is because it would be unreasonable to not have a default behavior for the fields of the object and to require the user to explicitly choose the correct treatment of each object field. A language claiming to support generic programming well requires a solution to this "unwanted default behavior" problem.

arnetheduck commented 3 years ago

This is because it would be unreasonable to not have a default behavior for the fields of the object

the fields should get serialized if there is a (explicitly enabled) serializer for the type of the field?