Crystal: Log::Context: Allow symbol

[caspiano]

Can we allow Symbol in the Log::Context datum?

[bcardiff]

Originally the keys were symbol. Buy in order to support dynamic key names, string need to be used.

How are you trying to use Context?

[Blacksmoke16]

@bcardiff I think he means as a value within the hash.

[caspiano]

The error arose when I attempted to set context with a NamedTuple containing a Symbol field.

require "log"

Log.context.set({type: :a, name: "apple"})

yields...

error in line 3
Error: instantiating 'Log::Context#set(NamedTuple(type: Symbol, name: String))'


In /usr/share/crystal/src/log/main.cr:134:56

 134 | extend_fiber_context(Fiber.current, Log::Context.new(values))
                                                        ^--
Error: instantiating 'Log::Context.class#new(NamedTuple(type: Symbol, name: String))'


In /usr/share/crystal/src/log/context.cr:15:11

 15 | tuple.each do |key, value|
            ^---
Error: instantiating 'NamedTuple(type: Symbol, name: String)#each()'


In /usr/share/crystal/src/log/context.cr:15:11

 15 | tuple.each do |key, value|
            ^---
Error: instantiating 'NamedTuple(type: Symbol, name: String)#each()'


In /usr/share/crystal/src/log/context.cr:16:23

 16 | raw[key.to_s] = to_context(value)
                      ^---------
Error: instantiating 'to_context(Symbol)'


In /usr/share/crystal/src/log/context.cr:48:44

 48 | value.is_a?(Context) ? value : Context.new(value)
                                             ^--
Error: no overload matches 'Log::Context.new' with type Symbol

Overloads are:
 - Log::Context.new(hash : Hash(String, V))
 - Log::Context.new(hash : Hash(Symbol, V))
 - Log::Context.new(raw : Type)
 - Log::Context.new(tuple : NamedTuple)
 - Log::Context.new(ary : Array)
 - Log::Context.new()

[caspiano]

See my temporary workaround

[straight-shoota]

What's the use case for symbol here? Why not just have a string?

[caspiano]

Principle of least astonishment? Symbols are not deprecated yet so I feel like people will still use them as values and the inability to pass a symbol to your Log context is a little unintuitive to me.

[straight-shoota]

This is not about whether symbols are legal or not, but whether there's a use case for that. As far as I can see, there's no reason to use a symbol in a log context.

[j8r]

Why the API also allows to have an Hash with symbols as keys, what a bad practice...
It encourages the use of symbols where it is not needed, and even though one can easily use hash.transform_keys &.to_s.

In this perspective, I can perfectly understand why one will ask why this is not allowed too:

Log.context.set({:type => :a, :name => :apple})

[Blacksmoke16]

Why the API also allows to have an Hash with symbols as keys

It converts them to string keys. https://github.com/crystal-lang/crystal/blob/master/src/log/context.cr#L39

[j8r]

Yeah I know that @Blacksmoke16 , just find this adds more confusion than usefulness.
"We don't recommend using symbols like that - use strings for keys instead, but we have an API for them in case you are stubborn". :smile:

[asterite]

@j8r I don't recommend them. Brian wrote the code and he likes symbols. I think for now it's not decided whether symbols are going to stay or leave.

[j8r]

@asterite Sure, no problem.
Just saying, extending the API to support Hash(Symbol, T) is not very good.

This construction is a Rubyism, not a Crystalism. grep -r "Hash(Symbol, " src/ returns only files in src/log/.

So, I propose to remove the mentions of Symbol in Log - String being recommended.

We are free to use any other types as keys, like Char, Path or String:

require "log"

Log.context.set({:type => "a", :name => "apple"}.transform_keys &.to_s)

Log.set(values : Hash(K, V)) forall K, V can be done, but I don't think it is a good idea. One won't know how the keys are transformed to strings.

[j8r]

Said differently: Hash(Symbol, T) should not be common - not supporting this construction is not very a problem (right?).

[waj]

When @bcardiff and me were thinking about the design of the log context, I initially though that the context should be a dictionary where the keys are symbols, and not strings. The reason for this is that I don't think there is a scenario where dynamic keys are necessary for the context. After all these context must be well known so they can be selectively rendered in text logs or used for filters. Also using symbols as the keys is much more performant than using strings, even literal strings. Now the content for these keys, should be at least something that doesn't cause trouble being converted to JSON, to make it easy to fit context values in logging services like Sentry, Appsignal, etc.

In other words, instead of removing references of Symbol I actually prefer to remove String as the base key for context entries. Of course the content of the entries could be hashes of any type that can be converted to JSON.

[straight-shoota]

@waj What you're suggesting is to treat the top level context differently from subcontexts? Like Log::Entry#context would be Hash(Symbol, Context) and Context contain Hash(String, Context) (not necessarily in the exact same way, but to get the idea).

[j8r]

I don't know what Symbol adds compared to String: both are not type safe.
There may have performance improvements, but actually they are converted to strings anyway.

As now, we can use NamedTuple and kwargs, Hash of Symbols looks superfluous (and IMO, uglier).

[waj]

@straight-shoota yes, something like that. Although I don't see any reason why the content of the context values couldn't contain hash with symbol keys.

@j8r I'm not sure what you mean with "type safe", but I guess you're talking about the inability to restrict the possible values for the context keys. Maybe right now symbols are being converted to strings, but what I'm saying is they shouldn't. Symbols are similar to strings in some sense, but they guarantee no new values are generated at runtime.

I'm not going to enter into the discussion beauty or ugliness of symbols, because I like them but that's a dead end topic 🙂

[straight-shoota]

Although I don't see any reason why the content of the context values couldn't contain hash with symbol keys.

Maybe because it doesn't have an effective benefit and is just another way to do more or less the same thing?

Also, there is a pending idea to eventually remove symbols from the language in favour of enums and strings. I'm not sure how realistic and imminent that is but it shows to have some support in the community and core team. There hasn't been a proper discussion yet and obviously this couldn't happen on short notice. But it would still be worth considering that and avoid new symbol based APIs when it's not strictly necessary. In case we decide to keep symbols in the language, it would be easy to add them to this API later.

[waj]

I know the idea of removing symbols comes and go sometimes, but I'm not fond to that. Enums replaced the usage of symbols in many places, but they are still great to define an unbounded set of atomic values. I don't see them disappearing anytime soon from the language and instead I want to encourage its use for these cases. Forcing string comparison when the values are well known, doesn't sound great to me, specially having a better suited type for this job.

[RX14]

Removing the ability to have dynamic keys is a no-go for me. Adding symbols as a possible value type is fine.

It wouldn't be so hard to make constant strings have the exact same performance as symbols, you'd just need to check that both sides of the equality operator are in the constants ELF section then compare by pointer. Not hard to do at all, and adding a few more instructions on a wide processor like x86 won't use any more cycles.

[waj]

That's already happening in Crystal. Literal strings are always the same reference and the comparison happens first by pointer. Still, comparing symbols is faster than comparing strings with the added guarantee that this comparison will never be affected by dynamic values because they just don't exist.

Can you give one valid example of acceptable usage of log context with dynamic values?

[bcardiff]

A reminder that the topic is about allowing or not Symbols as value. https://github.com/crystal-lang/crystal/issues/9159#issuecomment-618135803

---

Adding this should allow conversion of the Symbol to the String inside the context.

  def initialize(raw : Symbol)
    @raw = raw.to_s
  end

There are others conversions already when creating a context in order allow

Log.context.set foo: {bar: 1}

even if the named tuple {bar: 1} is not _the value type_ stored in foo. It is converted to a Log::Context with key string key "bar".

I'm fine with adding that to allow

Log.context.set fruit: :apple

---

Back to our regular String vs Symbols: I like to use Symbols :-)

[j8r]

@waj I meant, not need to use a dynamic Hash, a NamedTuple as elegant:

Log.context.set({abc: "def"})
Log.context.set({:abc => "def"})

I was not really talking if Symbol are good or bad, I meant here using them in a Hash as values.
If we want a pure static collection, NamedTuple is fine - we can even merge them.

[RX14]

Literal strings are always the same reference and the comparison happens first by pointer

Yes but if the comparison fails you always have to fall back to comparing the memory. If you can prove that both strings are in the readonly elf section then this fallback doesn't have to exist.

Can you give one valid example of acceptable usage of log context with dynamic values?

Logging all HTTP headers as context to a failed request sounds like a useful tool to me.

That's just a side note anyway, this PR is fine.