-
Notifications
You must be signed in to change notification settings - Fork 31
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
Remove #context and #global_context (and thus Thread locals)? #30
Comments
Well, I've been using scrolls as a structured logging solution for a fairly complex bit of backend. That means setting a context in one bit, and logging something quite a ways away from it without knowledge of the current context. It is rather convenient for that particular usecase, but I can certainly re-implement it outside of Scrolls. |
I have not yet starting using contexts, but it's something that I do really think is an important part of logging. For instance, I really want to tie all log messages related to a given user_id's web request to that user. I've been trying to think if there's a way you can abstract the context stuff so that you don't have to directly be writing to thread locals, but give people the opportunity to integrate that if that's how they want to deal with contexts, but I'm not sure it would be worth the effort to do that... |
There's always the option of making a logging object per request and pass that through to everything that needs to log with a specific context. With that in mind, logging context is probably one of the best cases for using thread-local variables, simply because of the simplicity it adds to the overall structure of the app. That said, context support could be made optional code, or an outright (trivial) plugin. |
The more I think about this the less I like the idea of removing #context. I don't like the thread locals for sure, but they are simple, and in most cases won't cause issues. The idea of making them optional or a plugin is interesting, but I am not sure we've run into a case where they have caused issues. Chalking this up to premature optimization, if a user doesn't use #context they won't have issues regardless, if they do, they can file a bug and we'll readdress the issue. I agree they are very useful for user_ids, request_ids and the like. KK, closing. Thanks everyone for the discussion. |
I am inclined to remove the ability to form a context. I am not a big fan of having to use Thread locals here and I think removing these simplify the library considerably.
My only reservation is whether people are using the method currently since I don't want to break existing code.
Please comment below whether you think this is a good idea or not.
The text was updated successfully, but these errors were encountered: