Repository navigation
Set up integrations after the app's initializers, and stop Shrine, AMS and Puma writing text lines - #18
Merged
Conversation
Apps usually load Shrine's instrumentation plugin in their own
initializer, which subscribes Shrine's default log subscriber: it writes
each upload, download, open, exists, delete and metadata event to
Shrine.logger as a line of text, next to LogStruct's structured line.
LogStruct only removed it when the plugin was already configured, and
that path listened on the wrong notification names ("shrine.upload"
instead of "upload.shrine"), unsubscribed every other listener for those
events and passed ActiveSupport events where Shrine's were expected.
LogStruct now loads the plugin without the default subscriber, subscribes
its own with Shrine.subscribe, and skips those events in Shrine's default
subscriber whenever the app loads it, before or after LogStruct. It also
points Shrine.logger at SemanticLogger, so other plugins' log lines and
Shrine's warnings are written as JSON instead of plain text.
AMS attaches a log subscriber that writes "Rendered UserSerializer with ActiveModelSerializers::Adapter::Attributes (0.1ms)" for every render, so each render was logged twice: once as that line of text and once as LogStruct's structured ams.render line.
The logstruct.setup initializer only had to run before the middleware stack was built, so it ran before the app's config/initializers. The documented place to configure LogStruct is config/initializers/ logstruct.rb, so its integration toggles were ignored, and filters that other initializers added to filter_parameters (such as Rails' default filter_parameter_logging.rb) were never merged into LogStruct's filters. It now runs after :load_config_initializers. `config.enabled` and `config.enabled_environments` set in an initializer were ignored too, because LogStruct decides whether it's enabled when it's required, and the logger is replaced as soon as Rails creates it. The logger is still replaced early, so boot-time logs and code that keeps a reference to Rails.logger use it, but setup now replaces it late if an initializer enabled LogStruct, and puts the original loggers back if an initializer disabled it. As documented, LOGSTRUCT_ENABLED takes precedence, then `enabled`, then `enabled_environments`.
Inside LogStruct::Integrations::GoodJob, `GoodJob` is that module, so the integration set the logger and thread error handler on itself and GoodJob kept its own logger. It now sets them on ::GoodJob. The thread error handler calls the handler the app set first (GoodJob's README sets one to report errors), then logs the error.
Integrations are now set up after config/initializers, so Sidekiq kept its own text logger while they ran. An initializer that configures Sidekiq logs with that logger straight away (connecting to Redis, setting up a fetcher), so those lines were plain text. The Railtie now replaces Sidekiq's logger as soon as it replaces Rails.logger, and puts Sidekiq's logger back if an initializer turns LogStruct or the Sidekiq integration off.
Puma::ErrorLogger writes request errors (an exception the app didn't handle, a malformed request, an SSL error) as a plain text line on STDERR: the time, the request line with its query string and client IP, and the error. LogStruct's Puma integration now logs them as error lines (src puma, evt error) with the error, method and path. The query string is left out, since tokens in it can't be filtered inside the line.
The puma lifecycle initializer wrote "[logstruct] puma lifecycle init" to STDERR on every rails server boot.
|
Warning Review limit reached
This review includes 19 billable files and costs up to $4.75.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 37 minutes for your next included review. View limit details
Comment |
This was referenced Oct 17, 2026
Merged
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Third of twelve stacked PRs for 0.2.0.
What this fixes
config/initializers/logstruct.rbwere ignored. Integrations were set up before the app's initializers ran, so the documented place to configure LogStruct had no effect on integration toggles, and filters other initializers added tofilter_parameterswere never merged. Setup now runs afterconfig/initializers.enabledandenabled_environmentsset in an initializer work too (LOGSTRUCT_ENABLEDstill takes precedence).Shrine.loggerwrites JSON.LogStruct::Integrations::GoodJob,GoodJobis that module, so the integration set the logger on itself. It now sets::GoodJob.logger(the thread error handling is reworked in the GoodJob PR).[logstruct] puma lifecycle init) Puma servers wrote to STDERR at boot.Because setup runs after the app's initializers, LogStruct's loggers and handlers now replace ones an initializer set (a custom Sidekiq logger, lograge settings). Configure those through LogStruct.
Testing
Every regression test fails without its fix. Sorbet and the unit suite (300 tests) pass at this branch's tip.
Stack for 0.2.0, merged in order: #16 → #17 → #18 → #19 → #20 → #21 → #22 → #23 → #24 → #25 → #26 → #27