Skip to content
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

Implement logger broadcasting #1357

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

unflxw
Copy link
Contributor

@unflxw unflxw commented Dec 19, 2024

This PR builds on top of the changes in #1355.

Docs need to be updated to recommend this over Rails.logger.broadcast_to: https://github.com/appsignal/appsignal-docs/pull/2315


Implement logger broadcasting

Implement broadcasting logs to other loggers, following the same
interface as #broadcast_to. Unlike #broadcast_to, however,
instead of forwarding each method call one-for-one, we rely on the
#add method which is expected of the logger interface.

As such, we should not run into the kinds of bugs that affect
ActiveSupport::BroadcastLogger when used alongside additional
logger features such as tagged logging, such as rails/rails#49745.

This enables customers to use both broadcast logging and tagged
logging with the AppSignal logger without running into
application-breaking bugs.

As a trade-off, unlike ActiveSupport::BroadcastLogger, our
broadcast implementation is sensitive to the order in which those
additional features are applied, expecting them to be applied on
top of the broadcast logger, and not on the underlying loggers:

 # Incorrect example
some_logger = ActiveSupport::TaggedLogging.new(
  Logger.new("/etc/app.log")
)

appsignal_logger = Appsignal::Logger.new("app")

 # Even if `some_logger` is a tagging-enabled logger,
 # this logger will not behave as a tagging-enabled logger.
Rails.logger = appsignal_logger.broadcast_to(some_logger)
 # Correct example
some_logger = Logger.new("/etc/app.log")
appsignal_logger = Appsignal::Logger.new("app")

appsignal_logger.broadcast_to(some_logger)
 # As the tagged logging is applied on the broadcast logger,
 # both logs sent to AppSignal and to `some_logger` will include
 # given tags, and `Rails.logger` will be a tagging-enabled logger.
Rails.logger = ActiveSupport::TaggedLogging.new(appsignal_logger)

This commit removes tagged logging support from `Appsignal::Logger`,
essentially reverting most of the changes in #1344, #1350, #1351 and

The change to use instance variables instead of thread variables is
maintained, as well as the tests that assert the expected behaviour
when used alongside `ActiveSupport::TaggedLogging`, which pass
notwithstanding the reverted functionality. This makes explicit in
our test suite the way in which tagged logging is supported.
@unflxw unflxw added the chore label Dec 19, 2024
@unflxw unflxw self-assigned this Dec 19, 2024
@unflxw unflxw force-pushed the implement-logger-broadcasting branch from 8d31106 to 160320f Compare December 19, 2024 14:21
Implement broadcasting logs to other loggers, following the same
interface as `#broadcast_to`. Unlike `#broadcast_to`, however,
instead of forwarding each method call one-for-one, we rely on the
`#add` method which is expected of the logger interface.

As such, we should not run into the kinds of bugs that affect
`ActiveSupport::BroadcastLogger` when used alongside additional
logger features such as tagged logging, such as rails/rails#49745.

This enables customers to use both broadcast logging and tagged
logging with the AppSignal logger without running into
application-breaking bugs.

As a trade-off, unlike `ActiveSupport::BroadcastLogger`, our
broadcast implementation is sensitive to the order in which those
additional features are applied, expecting them to be applied on
top of the broadcast logger, and not on the underlying loggers:

```ruby
 # Incorrect example
some_logger = ActiveSupport::TaggedLogging.new(
  Logger.new("/etc/app.log")
)

appsignal_logger = Appsignal::Logger.new("app")

 # Even if `some_logger` is a tagging-enabled logger,
 # this logger will not behave as a tagging-enabled logger.
Rails.logger = appsignal_logger.broadcast_to(some_logger)
```

```ruby
 # Correct example
some_logger = Logger.new("/etc/app.log")
appsignal_logger = Appsignal::Logger.new("app")

appsignal_logger.broadcast_to(some_logger)
 # As the tagged logging is applied on the broadcast logger,
 # both logs sent to AppSignal and to `some_logger` will include
 # given tags, and `Rails.logger` will be a tagging-enabled logger.
Rails.logger = ActiveSupport::TaggedLogging.new(appsignal_logger)
```
@unflxw unflxw force-pushed the implement-logger-broadcasting branch from 160320f to 4a7adfb Compare December 19, 2024 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant