crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.42k stars 1.62k forks source link

Log.emiter does not work with a Union type #10689

Open erdnaxeli opened 3 years ago

erdnaxeli commented 3 years ago

Hi,

With this code

require "log"

a = 2
if a % 2 == 0
  a = 0_u8
end

pp typeof(a)  # => (Int32 | UInt8)
Log.info &.emit("test", a: a)

I got the following error:

Showing last frame. Use --error-trace for full trace.

In /usr/share/crystal/src/log/metadata.cr:217:42

 217 | value.is_a?(Value) ? value : Value.new(value)
                                          ^--
Error: no overload matches 'Log::Metadata::Value.new' with type (Int32 | UInt8)

Overloads are:
 - Log::Metadata::Value.new(raw : Type)
 - Log::Metadata::Value.new(hash : NamedTuple | Hash)
 - Log::Metadata::Value.new(ary : Array)
Couldn't find overloads for these types:
 - Log::Metadata::Value.new(UInt8)

I see by diging in the doc that it exists Log::Metadata::Value::Type that define all supported types but:

straight-shoota commented 3 years ago

This is not caused by a union type, but UInt8 is not supported:

Log.info &.emit("test", a: 0_u8) # Error: no overload matches 'Log::Metadata::Value.new' with type UInt8

The log metadata is represented as a relatively simple data structure which supports only a hand full of primitive data types for values. You'll have to convert the number to Int32 to pass it as log metadata. Or use some other form of serialization.

bcardiff commented 3 years ago

The supported types are the ones listed in https://crystal-lang.org/api/1.0.0/Log/Metadata/Value/Type.html . The actionable here is to improve the docs of emit. It is not expected for the Log::Metadata to handle arbitrary types.

erdnaxeli commented 3 years ago

Well, neither Union nor UInt8 feels like arbitrary types to me :p. Maybe we should add for all primitive types (U)Int?

straight-shoota commented 3 years ago

It is very arbitrary. For logging purposes, there's not much reason to preserve specific integer types. Just convert it to Int32.