Fix internal logger in Capistrano tasks and remove Config logger instance #1306
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The transmitter logged partially through the `Appsignal.internal_logger` and the logger from the Config class. Make this consistent, like any other place in the gem, by logging via `Appsignal.internal_logger`.
The loggers given to the Config class in the Capistrano tasks write to a StringIO which is never read afterwards. Call `_start_logger` to initialize the logger and write the buffered log messages either to STDOUT or the AppSignal log file after the config has loaded.
Directly access the `Appsignal.internal_logger` method instead of the Config class having its own logger instance that could be different from the configured logger.
tombruijn
force-pushed
the
refactor-logger
branch
from
8fca515 to
e671a35
Compare
tombruijn
changed the title
Follow up on the previous commits to remove the logger instance from the Config class, which was set by Capistrano. It didn't actually log anything before, as it wrote to a `StringIO.new`. Log now to the configured logger based on the application's config. I see no harm in making the logger work. By default it will log to the `appsignal.log` file. If apps have configured it to log to stdout, they have deliberately configured that so there should be no problem.
luismiramirez
approved these changes
Sep 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use internal logger from in transmitter
The transmitter logged partially through the
Appsignal.internal_loggerand the logger from the Config class. Make this consistent, like any other place in the gem, by logging viaAppsignal.internal_logger.Fix Capistrano tasks not logging anything
The loggers given to the Config class in the Capistrano tasks write to a StringIO which is never read afterwards. Call
_start_loggerto initialize the logger and write the buffered log messages either to STDOUT or the AppSignal log file after the config has loaded.Remove logger from config class
Directly access the
Appsignal.internal_loggermethod instead of the Config class having its own logger instance that could be different from the configured logger.Assert the logger works in Capistrano
Follow up on the previous commits to remove the logger instance from the Config class, which was set by Capistrano.
It didn't actually log anything before, as it wrote to a
StringIO.new. Log now to the configured logger based on the application's config.I see no harm in making the logger work. By default it will log to the
appsignal.logfile. If apps have configured it to log to stdout, they have deliberately configured that so there should be no problem.