-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Log::Context: Allow symbol #9159
Comments
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? |
@bcardiff I think he means as a value within the hash. |
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...
|
See my temporary workaround |
What's the use case for symbol here? Why not just have a string? |
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. |
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. |
Why the API also allows to have an Hash with symbols as keys, what a bad practice... In this perspective, I can perfectly understand why one will ask why this is not allowed too: Log.context.set({:type => :a, :name => :apple}) |
It converts them to string keys. https://github.com/crystal-lang/crystal/blob/master/src/log/context.cr#L39 |
Yeah I know that @Blacksmoke16 , just find this adds more confusion than usefulness. |
@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. |
@asterite Sure, no problem. This construction is a Rubyism, not a Crystalism. So, I propose to remove the mentions of We are free to use any other types as keys, like require "log"
Log.context.set({:type => "a", :name => "apple"}.transform_keys &.to_s)
|
Said differently: |
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 |
@waj What you're suggesting is to treat the top level context differently from subcontexts? Like |
I don't know what As now, we can use |
@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 🙂 |
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. |
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. |
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. |
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? |
A reminder that the topic is about allowing or not Symbols as value. #9159 (comment) 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 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 :-) |
@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 |
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.
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. |
Can we allow
Symbol
in theLog::Context
datum?The text was updated successfully, but these errors were encountered: