Configure server side logging #844
Conversation
## Walkthrough
The changes introduce enhancements and refactoring to logging and configuration handling in the Shoryuken codebase. These include improved logger delegation, explicit adapter inheritance for Active Job integration, a refactored logging formatter hierarchy with a new base class and additional formatter, and memoization of the logger instance within the options class.
## Changes
| File(s) | Change Summary |
|------------------------------------------------|-----------------------------------------------------------------------------------------------------------------|
| lib/shoryuken.rb | Added `:logger=` to delegated methods for `shoryuken_options` in the `Shoryuken` module. |
| lib/shoryuken/extensions/active_job_adapter.rb | Added conditional `BaseAdapter` class; updated `ShoryukenAdapter` to inherit from `BaseAdapter`. |
| lib/shoryuken/logging.rb | Introduced `Base` formatter class; updated `Pretty` to inherit from `Base`; added `WithoutTimestamp` formatter. |
| lib/shoryuken/options.rb | Added `attr_writer :logger`; reformatted attribute accessors; memoized logger instance in `Options` class. |
| gemfiles/rails_6_1.gemfile | Removed fixed version constraint from `pry-byebug` gem declaration. |
| gemfiles/rails_7_0.gemfile | Removed `pry` gem; removed version constraints from `pry-byebug` gem declaration. |
| shoryuken.gemspec | Added new runtime dependency on `mutex_m` gem. |
## Poem
> In the warren logs now shine anew,
> With formatters crisp and options true.
> A logger passed, a thread ID found,
> The adapters now on solid ground.
> Rabbits cheer for code refined—
> In every hop, new gems we find!
> 🐇✨✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
BenMorganMY
force-pushed
the
configure-server-side-logging
branch
from
152f230 to
f6f7683
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/shoryuken/logging.rb (1)
8-8: Minor style improvement: Use single quotes for simple string literal.The string
"sidekiq_tid"doesn't require interpolation, so single quotes would be more appropriate per Ruby style guidelines.- Thread.current["sidekiq_tid"] ||= (Thread.current.object_id ^ ::Process.pid).to_s(36) + Thread.current['sidekiq_tid'] ||= (Thread.current.object_id ^ ::Process.pid).to_s(36)Note: The use of
'sidekiq_tid'as the key name appears intentional for compatibility with Sidekiq's thread identification system, which is a reasonable design choice for consistency across similar libraries.🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 8-8: Prefer single-quoted strings when you don't need string interpolation or special symbols.
(Style/StringLiterals)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/shoryuken.rb(1 hunks)lib/shoryuken/extensions/active_job_adapter.rb(1 hunks)lib/shoryuken/logging.rb(2 hunks)lib/shoryuken/options.rb(2 hunks)
🧰 Additional context used
🪛 RuboCop (1.75.5)
lib/shoryuken/logging.rb
[convention] 8-8: Prefer single-quoted strings when you don't need string interpolation or special symbols.
(Style/StringLiterals)
lib/shoryuken/options.rb
[convention] 21-21: Align the arguments of a method call if they span more than one line.
(Layout/ArgumentAlignment)
[convention] 22-22: Align the arguments of a method call if they span more than one line.
(Layout/ArgumentAlignment)
[convention] 23-23: Align the arguments of a method call if they span more than one line.
(Layout/ArgumentAlignment)
🔇 Additional comments (7)
lib/shoryuken.rb (1)
78-78: LGTM! Clean implementation of logger setter delegation.The addition of
:logger=to the delegated methods perfectly complements the existing:loggergetter and enables the core functionality described in the PR objectives. This allows users to set a custom logger viaShoryuken.logger = custom_loggeras intended.lib/shoryuken/extensions/active_job_adapter.rb (1)
17-17: Excellent improvement! Explicit inheritance enhances code clarity.Making the inheritance from
ActiveJob::QueueAdapters::AbstractAdapterexplicit is a best practice that clearly establishes the adapter's relationship with the ActiveJob framework. This improves code maintainability and follows ActiveJob's recommended adapter pattern.lib/shoryuken/options.rb (2)
20-25: Well-structured attribute organization and logger configuration support.The reformatting of the
attr_accessordeclarations improves readability, and the addition of:loggertoattr_writerenables the custom logger configuration feature. This change works seamlessly with the delegation in the main module.🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 21-21: Align the arguments of a method call if they span more than one line.
(Layout/ArgumentAlignment)
[convention] 22-22: Align the arguments of a method call if they span more than one line.
(Layout/ArgumentAlignment)
[convention] 23-23: Align the arguments of a method call if they span more than one line.
(Layout/ArgumentAlignment)
101-101: Perfect implementation of logger memoization.The memoization pattern
@logger ||= Shoryuken::Logging.loggerensures that once a custom logger is set via the writer method, it will be preserved and returned on subsequent calls. This is exactly the behavior needed for the configurable logger feature.lib/shoryuken/logging.rb (3)
6-15: Excellent refactoring! Clean base class for shared formatter functionality.The introduction of the
Baseclass that inherits from::Logger::Formatterprovides a solid foundation for formatter implementations. The sharedtidandcontextmethods eliminate code duplication and create a consistent interface.🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 8-8: Prefer single-quoted strings when you don't need string interpolation or special symbols.
(Style/StringLiterals)
17-22: Good refactoring of Pretty formatter to use base class.The
Prettyformatter now leverages the sharedtidmethod from the base class, which improves maintainability and consistency across formatters.
24-28: Great addition! WithoutTimestamp formatter provides Sidekiq feature parity.The new
WithoutTimestampformatter perfectly aligns with the PR objective of providing feature parity with Sidekiq's logging capabilities. The formatpid=#{Process.pid} tid=#{tid}#{context} #{severity}: #{message}is clean and informative without the timestamp.
BenMorganMY
force-pushed
the
configure-server-side-logging
branch
from
f6f7683 to
dd2334b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/shoryuken/options.rb (1)
20-24: Fix argument alignment to comply with RuboCop conventions.The reformatted
attr_accessordeclarations improve readability, but the alignment doesn't follow RuboCop conventions.Apply this diff to fix the alignment:
- attr_accessor :active_job_queue_name_prefixing, :cache_visibility_timeout, - :groups, :launcher_executor, :reloader, :enable_reloading, - :start_callback, :stop_callback, :worker_executor, :worker_registry, - :exception_handlers + attr_accessor :active_job_queue_name_prefixing, :cache_visibility_timeout, + :groups, :launcher_executor, :reloader, :enable_reloading, + :start_callback, :stop_callback, :worker_executor, :worker_registry, + :exception_handlers🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 21-21: Align the arguments of a method call if they span more than one line.
(Layout/ArgumentAlignment)
[convention] 22-22: Align the arguments of a method call if they span more than one line.
(Layout/ArgumentAlignment)
[convention] 23-23: Align the arguments of a method call if they span more than one line.
(Layout/ArgumentAlignment)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/shoryuken.rb(1 hunks)lib/shoryuken/extensions/active_job_adapter.rb(1 hunks)lib/shoryuken/logging.rb(2 hunks)lib/shoryuken/options.rb(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/shoryuken/extensions/active_job_adapter.rb
- lib/shoryuken.rb
- lib/shoryuken/logging.rb
🧰 Additional context used
🪛 RuboCop (1.75.5)
lib/shoryuken/options.rb
[convention] 21-21: Align the arguments of a method call if they span more than one line.
(Layout/ArgumentAlignment)
[convention] 22-22: Align the arguments of a method call if they span more than one line.
(Layout/ArgumentAlignment)
[convention] 23-23: Align the arguments of a method call if they span more than one line.
(Layout/ArgumentAlignment)
🔇 Additional comments (2)
lib/shoryuken/options.rb (2)
25-25: LGTM! Excellent addition for configurable logging.The
attr_writer :loggerenables external assignment of custom loggers, which perfectly aligns with the PR objective to provide feature parity with Sidekiq's logging capabilities.
100-102:❌ Incorrect review comment
Consider thread safety for the memoization pattern.
The logger memoization implementation correctly allows custom loggers to be cached and reused. However, the
@logger ||= valuepattern is not thread-safe in Ruby and could lead to race conditions in multi-threaded environments.Consider using a thread-safe memoization approach:
- def logger - @logger ||= Shoryuken::Logging.logger - end + def logger + return @logger if defined?(@logger) + @logger_mutex ||= Mutex.new + @logger_mutex.synchronize do + @logger ||= Shoryuken::Logging.logger + end + endOr verify if Shoryuken's architecture ensures single-threaded access to the Options instance during logger configuration.
🏁 Script executed:
#!/bin/bash # Search for concurrent access patterns to Options class rg -A 3 -B 3 "Options\.new|\.logger\s*=" --type rubyLength of output: 620
🏁 Script executed:
#!/bin/bash # Display the start of the Options class including the logger method context sed -n '1,200p' lib/shoryuken/options.rbLength of output: 5105
No thread‐safety issue with
Options#loggerThe
Optionsinstance is created and fully configured on the main thread before any worker threads start, so there’s no concurrent access to@logger. In practice, even if two threads did race in the||=assignment, they’d both cache the sameShoryuken::Logging.loggerinstance—and the worst‐case is a redundant initializer call, not a broken state.Moreover, wrapping
Options#loggerin aMutexwouldn’t fix the underlying pattern inShoryuken::Logging.logger, which uses the same||=memoization. If you really need thread‐safe memoization, you’d have to synchronize insideShoryuken::Logging.loggeritself.No changes required here.
Likely an incorrect or invalid review comment.
|
Thanks. Sorry I missed that 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
lib/shoryuken/extensions/active_job_adapter.rb (1)
80-86: Missingrequire 'digest'can triggerNameError
Digest::SHA256is referenced without ensuring thedigeststdlib is loaded. In minimal Ruby environments the constant will be absent, raisingNameErrorat runtime.+require 'digest' # needed for Digest::SHA256 belowPlace it near the other top-level requires.
🧹 Nitpick comments (1)
lib/shoryuken/extensions/active_job_adapter.rb (1)
9-13: Minor RuboCop style nit – prefer single quotesRuboCop flags the double-quoted string
'6.1.0'where interpolation isn’t used:- if ActiveJob.version > Gem::Version.new("6.1.0") + if ActiveJob.version > Gem::Version.new('6.1.0')Purely cosmetic but keeps the codebase consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/shoryuken/extensions/active_job_adapter.rb(2 hunks)
🧰 Additional context used
🪛 RuboCop (1.75.5)
lib/shoryuken/extensions/active_job_adapter.rb
[convention] 9-9: Prefer single-quoted strings when you don't need string interpolation or special symbols.
(Style/StringLiterals)
An error occurred while loading ./spec/shoryuken/extensions/active_job_base_spec.rb. - Did you mean?
rspec ./spec/shoryuken/extensions/active_job_wrapper_spec.rb
rspec ./spec/shoryuken/extensions/active_job_adapter_spec.rb
Failure/Error: require 'active_job'
LoadError:
cannot load such file -- mutex_m
# ./spec/shoryuken/extensions/active_job_base_spec.rb:2:in '<top (required)>'
|
@mensfeld can you take another look at this? |
|
If all except 6.1 pass I will retarget and remove 6.1 support. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/shoryuken/extensions/active_job_adapter.rb (1)
8-8: Use single quotes for string literals.The string doesn't require interpolation or special symbols, so single quotes are preferred by Ruby style conventions.
- if ActiveJob.version >= Gem::Version.new("7.0.0") + if ActiveJob.version >= Gem::Version.new('7.0.0')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
gemfiles/rails_6_1.gemfile(1 hunks)gemfiles/rails_7_0.gemfile(1 hunks)lib/shoryuken/extensions/active_job_adapter.rb(2 hunks)shoryuken.gemspec(1 hunks)
🧰 Additional context used
🪛 RuboCop (1.75.5)
gemfiles/rails_7_0.gemfile
[convention] 14-14: Prefer single-quoted strings when you don't need string interpolation or special symbols.
(Style/StringLiterals)
gemfiles/rails_6_1.gemfile
[convention] 14-14: Prefer single-quoted strings when you don't need string interpolation or special symbols.
(Style/StringLiterals)
lib/shoryuken/extensions/active_job_adapter.rb
[convention] 8-8: Prefer single-quoted strings when you don't need string interpolation or special symbols.
(Style/StringLiterals)
🪛 GitHub Actions: Specs
lib/shoryuken/extensions/active_job_adapter.rb
[error] 9-9: NameError: uninitialized constant ActiveJob::QueueAdapters::AbstractAdapter. This caused multiple spec files to fail loading.
🔇 Additional comments (3)
lib/shoryuken/extensions/active_job_adapter.rb (1)
23-23: Excellent approach for version-agnostic inheritance.The change to inherit from
BaseAdapterprovides clean version compatibility across different ActiveJob versions. This aligns well with the PR objectives of improving integration capabilities.gemfiles/rails_7_0.gemfile (1)
14-14: Fix string quote style and verify the relevance of Gemfile changes.The RuboCop hint suggests using single quotes for consistency. However, more importantly, this Gemfile change appears unrelated to the PR's stated objective of "Configure server side logging" for Shoryuken.
Apply this diff to fix the string quote style:
- gem "pry-byebug" + gem 'pry-byebug'Please verify that these Gemfile changes are intentionally part of this logging configuration PR, as they appear to be unrelated development dependency updates.
Likely an incorrect or invalid review comment.
gemfiles/rails_6_1.gemfile (1)
14-14: Fix string quote style and verify the relevance of Gemfile changes.Similar to the Rails 7.0 Gemfile, this change removes version constraints from development dependencies but appears unrelated to the PR's logging configuration objective.
Apply this diff to fix the string quote style:
- gem "pry-byebug" + gem 'pry-byebug'These Gemfile changes across both Rails versions suggest they may be incidental updates rather than core changes for the logging configuration feature.
Likely an incorrect or invalid review comment.
| if ActiveJob.version >= Gem::Version.new("7.0.0") | ||
| class BaseAdapter < ActiveJob::QueueAdapters::AbstractAdapter; end | ||
| else | ||
| class BaseAdapter; end | ||
| end |
There was a problem hiding this comment.
Fix the AbstractAdapter availability issue causing pipeline failures.
The pipeline is failing with NameError: uninitialized constant ActiveJob::QueueAdapters::AbstractAdapter even when the version check passes. This suggests AbstractAdapter may not be autoloaded when this code executes.
Consider a more defensive approach that safely checks for the constant's existence:
- if ActiveJob.version >= Gem::Version.new("7.0.0")
- class BaseAdapter < ActiveJob::QueueAdapters::AbstractAdapter; end
- else
- class BaseAdapter; end
- end
+ if ActiveJob.version >= Gem::Version.new("7.0.0") &&
+ defined?(ActiveJob::QueueAdapters::AbstractAdapter)
+ class BaseAdapter < ActiveJob::QueueAdapters::AbstractAdapter; end
+ else
+ class BaseAdapter; end
+ endAlternatively, you could wrap the inheritance in a begin/rescue block to handle cases where the constant is not available:
+ begin
+ if ActiveJob.version >= Gem::Version.new("7.0.0")
+ class BaseAdapter < ActiveJob::QueueAdapters::AbstractAdapter; end
+ else
+ class BaseAdapter; end
+ end
+ rescue NameError
+ class BaseAdapter; end
+ end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ActiveJob.version >= Gem::Version.new("7.0.0") | |
| class BaseAdapter < ActiveJob::QueueAdapters::AbstractAdapter; end | |
| else | |
| class BaseAdapter; end | |
| end | |
| if ActiveJob.version >= Gem::Version.new("7.0.0") && | |
| defined?(ActiveJob::QueueAdapters::AbstractAdapter) | |
| class BaseAdapter < ActiveJob::QueueAdapters::AbstractAdapter; end | |
| else | |
| class BaseAdapter; end | |
| end |
🧰 Tools
🪛 RuboCop (1.75.5)
[convention] 8-8: Prefer single-quoted strings when you don't need string interpolation or special symbols.
(Style/StringLiterals)
🪛 GitHub Actions: Specs
[error] 9-9: NameError: uninitialized constant ActiveJob::QueueAdapters::AbstractAdapter. This caused multiple spec files to fail loading.
🤖 Prompt for AI Agents
In lib/shoryuken/extensions/active_job_adapter.rb around lines 8 to 12, the code
assumes ActiveJob::QueueAdapters::AbstractAdapter is always defined when
ActiveJob version is 7.0.0 or higher, causing a NameError if it's not
autoloaded. To fix this, modify the code to first check if the AbstractAdapter
constant is defined before inheriting from it, or wrap the inheritance in a
begin/rescue block to fallback safely if the constant is missing, ensuring the
class definition does not raise an error.
| @@ -22,6 +22,7 @@ Gem::Specification.new do |spec| | |||
| spec.add_development_dependency 'rake' | |||
| spec.add_development_dependency 'rspec' | |||
|
|
|||
| spec.add_dependency 'mutex_m' | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Redundant dependency on mutex_m – please confirm necessity
mutex_m has been bundled with Ruby’s stdlib/default-gem set for many years (simply require 'mutex_m' works out of the box on ≥3.0). Declaring it as an external runtime dependency forces an extra gem install and introduces the risk of version drift or resolution failures in minimal images.
- spec.add_dependency 'mutex_m'
+# `mutex_m` ships with Ruby; no separate gem needed unless you target a Ruby build
+# that deliberately omits default gems.Unless you have evidence of environments where the default gem is absent, consider removing this line.
🏁 Script executed:
#!/bin/bash
set -e
# Locate the gemspec file
gemspec_path=$(fd shoryuken.gemspec | head -n1)
echo "Found gemspec: $gemspec_path"
echo -e "\n--- Start of gemspec (first 200 lines) ---"
sed -n '1,200p' "$gemspec_path"
echo -e "\n--- Ruby version requirement ---"
grep -n "required_ruby_version" "$gemspec_path" || echo "No required_ruby_version specified"
echo -e "\n--- Dependency list ---"
grep -n "add_dependency" "$gemspec_path"Length of output: 1827
Remove redundant mutex_m dependency
Since this gem already requires Ruby ≥3.0 (where mutex_m ships as a default stdlib gem), you don’t need to declare it as an external runtime dependency. Please remove the following line:
• File shoryuken.gemspec, line 25:
- spec.add_dependency 'mutex_m'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| spec.add_dependency 'mutex_m' |
🤖 Prompt for AI Agents
In shoryuken.gemspec at line 25, remove the line that adds 'mutex_m' as a
dependency because Ruby 3.0 and above include 'mutex_m' as a default standard
library, making this declaration redundant.
|
yeah let me merge it there and deal with this separately |
mensfeld
merged commit 0c1c09f
into
ruby-shoryuken:server-side-logging
* Configure server side logging (#844) * add the ability to be able to set the logger * add a without timestamp class * shoryuken adapter extends from active job queue adapter * use shoryuken_tid instead * ensure the abstract adapter is loaded * use a conditional base adapter to resolve rails 6.1 not having an abstract adapter * use active job version * appraisal install * fix issue with mutex_m not being loaded An error occurred while loading ./spec/shoryuken/extensions/active_job_base_spec.rb. - Did you mean? rspec ./spec/shoryuken/extensions/active_job_wrapper_spec.rb rspec ./spec/shoryuken/extensions/active_job_adapter_spec.rb Failure/Error: require 'active_job' LoadError: cannot load such file -- mutex_m # ./spec/shoryuken/extensions/active_job_base_spec.rb:2:in '<top (required)>' * fix version comparison and no need to load abstract adapter explicitly --------- Co-authored-by: Maciej Mensfeld <[email protected]> * remove rails 6.1 and ruby 3.0 support * use directly the aj adapter --------- Co-authored-by: Ben A. Morgan <[email protected]>
This PR allows Shoryuken to be able to configure the logger such as:
This provides some feature parity to Sidekiq and helps developers that are using a service like AppSignal get their logs up there.
https://docs.appsignal.com/logging/platforms/integrations/ruby.html#sidekiq-logger
Summary by CodeRabbit