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

Fix TaggedLogging to allow loggers to be instantiated multiple times without having to share the stack of tags #9065

Merged
merged 1 commit into from Jan 2, 2015

Conversation

Projects
None yet
6 participants
@atombender
Contributor

atombender commented Jan 24, 2013

This is accomplished by using a unique key for the thread-local tag list. Fixes #9064.

Fix TaggedLogging to allow loggers to be instantiated multiple times …
…without having to share the stack of tags. This is accomplished by using a unique key for the thread-local tag list. Fixes #9064.
@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Jan 24, 2013

Member

Looks good. Having a thread-local formatter might feel nicer, but it's a hassle to implement. /cc @tenderlove

Member

jeremy commented Jan 24, 2013

Looks good. Having a thread-local formatter might feel nicer, but it's a hassle to implement. /cc @tenderlove

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jan 24, 2013

Member

Don't we have Thread local configs now, that could be used for it?

Member

carlosantoniodasilva commented Jan 24, 2013

Don't we have Thread local configs now, that could be used for it?

@atombender

This comment has been minimized.

Show comment
Hide comment
@atombender

atombender Jan 24, 2013

Contributor

@carlosantoniodasilva: The tagged logger already uses a thread-local. The problem is that it acts as if there can only be a single tagged logger instance.

Here's my example from the previous issue (sorry about the dupe, didn't know Github listed pull requests in the issue tracker):

my_logger = ActiveSupport::TaggedLogging.new(Rails.logger)
my_logger.push_tags "my_tag"
my_logger.info "test"  # outputs "[my_tag] [my_tag] test"
Rails.logger.info "test"  # outputs "[my_tag] test"

It's also not safe to wrap a unique logger (ie., one that does not delegate to a logger shared around in the app, like Rails.logger):

my_logger = ActiveSupport::TaggedLogging.new(Logger.new($stdout))
my_logger.push_tags "my_tag"
Rails.logger.info "test"  # outputs "[my_tag] test"
Rails.logger.tagged("other_tag") do
  my_logger.info "test"  # outputs "[my_tag] [other_tag]"
end

In other words, TaggedLogging is effectively a singleton within a single thread.

The fix uses a unique key to store the thread-local tag list. (The only weakness is that the thread-local key will never be garbage collected until the thread dies, but it seems unrealistic that anyone would create a huge number of loggers.)

Contributor

atombender commented Jan 24, 2013

@carlosantoniodasilva: The tagged logger already uses a thread-local. The problem is that it acts as if there can only be a single tagged logger instance.

Here's my example from the previous issue (sorry about the dupe, didn't know Github listed pull requests in the issue tracker):

my_logger = ActiveSupport::TaggedLogging.new(Rails.logger)
my_logger.push_tags "my_tag"
my_logger.info "test"  # outputs "[my_tag] [my_tag] test"
Rails.logger.info "test"  # outputs "[my_tag] test"

It's also not safe to wrap a unique logger (ie., one that does not delegate to a logger shared around in the app, like Rails.logger):

my_logger = ActiveSupport::TaggedLogging.new(Logger.new($stdout))
my_logger.push_tags "my_tag"
Rails.logger.info "test"  # outputs "[my_tag] test"
Rails.logger.tagged("other_tag") do
  my_logger.info "test"  # outputs "[my_tag] [other_tag]"
end

In other words, TaggedLogging is effectively a singleton within a single thread.

The fix uses a unique key to store the thread-local tag list. (The only weakness is that the thread-local key will never be garbage collected until the thread dies, but it seems unrealistic that anyone would create a huge number of loggers.)

@ches

This comment has been minimized.

Show comment
Hide comment
@ches

ches Sep 12, 2013

👍

I wanted this behavior and expected it to just work. Particularly because current_tags is undocumented and was private in 3.2, one had to go to the source to see why it doesn't. I smacked my head when I got there.

ches commented Sep 12, 2013

👍

I wanted this behavior and expected it to just work. Particularly because current_tags is undocumented and was private in 3.2, one had to go to the source to see why it doesn't. I smacked my head when I got there.

rafaelfranca added a commit that referenced this pull request Jan 2, 2015

Merge pull request #9065 from atombender/master
Fix TaggedLogging to allow loggers to be instantiated multiple times without having to share the stack of tags

@rafaelfranca rafaelfranca merged commit eed68fd into rails:master Jan 2, 2015

@stephankaag

This comment has been minimized.

Show comment
Hide comment
@stephankaag

stephankaag commented Nov 5, 2015

@rafaelfranca Was this change ever released? Looks like it's not included in ActiveSupport 4.2.

https://github.com/rails/rails/blob/4-2-stable/activesupport/lib/active_support/tagged_logging.rb#L46

@atombender

This comment has been minimized.

Show comment
Hide comment
@atombender

atombender Nov 5, 2015

Contributor

@stephankaag: No, for some reason it's still only on master, even though it was merged more than 10 months ago. Not sure how the release process works, but I'm assuming it will end up in 4.3 (or 5.0).

Contributor

atombender commented Nov 5, 2015

@stephankaag: No, for some reason it's still only on master, even though it was merged more than 10 months ago. Not sure how the release process works, but I'm assuming it will end up in 4.3 (or 5.0).

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Jun 11, 2016

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Jun 13, 2016

rafaelfranca added a commit that referenced this pull request Jun 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment