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.

Sign up for GitHub

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

Jump to bottom

WIP: Overhaul some behaviors of this gem #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

WIP: Overhaul some behaviors of this gem #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions lib/logstasher.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
require 'logger'
require 'logstash-event'
require "logger"
require "logstash-event"

module LogStasher
class << self
SILENT_LOGGER = ::Logger.new("/dev/null", level: :unknown)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this constant be inside the `class << self``?

Being inside that means I can't refer to it normally like LogStasher::SILENT_LOGGER . I would have to write LogStasher.singleton_class::SILENT_LOGGER


attr_reader :append_fields_callback
attr_writer :enabled
attr_writer :include_parameters
Expand Down Expand Up @@ -61,6 +63,9 @@ def log_as_json(payload, as_logstash_event: false)
end

def logger
# Return a /dev/null logger if logstasher is enabled or silencing was enabled.
return SILENT_LOGGER if !enabled? || silence_standard_logging?
Copy link

@sshock sshock Jan 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't check silence_standard_logging? here, because then it kills direct logging too.


@logger ||= initialize_logger
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid we can't change this without potentially breaking someone, but I really think initialize_logger should be named default_logger or backup_logger or something and ought to be private.

If anyone is actually calling it from outside this class, due to its name they might think calling it actually changes @logger, which it doesn't.

So..., given that, maybe it is a good thing if renaming that and making it private breaks anyone who was trying to call it.

Note also that as part of making this private, it doesn't really need configurable params.

end

Expand All @@ -78,4 +83,4 @@ def silence_standard_logging?
end
end

require 'logstasher/railtie' if defined?(Rails)
require "logstasher/railtie" if defined?(Rails)
2 changes: 2 additions & 0 deletions lib/logstasher/device.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ def self.factory(config)
::LogStasher::Device::UDP.new(config)
when "stdout", :stdout then
$stdout
when "file", :file then
config["path"]
else
fail ArgumentError, "Unknown type: #{type}"
end
Expand Down
35 changes: 5 additions & 30 deletions lib/logstasher/railtie.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,45 +16,20 @@ class Railtie < ::Rails::Railtie
::LogStasher.include_parameters = options.include_parameters
::LogStasher.serialize_parameters = options.serialize_parameters
::LogStasher.silence_standard_logging = options.silence_standard_logging
::LogStasher.logger = options.logger || default_logger
::LogStasher.logger = options.logger
Copy link

@sshock sshock Jan 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure the order of execution of these three places:

  1. The config/environments/*.rb file in the consuming project.
  2. The config/initializers/logstasher.rb in the consuming project.
  3. This railtie.rb file here.

Just in case 1 or 2 could happen before 3, should we change = to ||= here to avoid a problem?

::LogStasher.logger ||= options.logger

That way anyone who explicitly sets ::LogStasher.logger from 1 or 2 won't have to worry about it getting overridden here.

(If you are confident that this file will always get executed earlier than 1 or 2, then we could leave it as is.)

::LogStasher.logger.level = options.log_level
::LogStasher.metadata = options.metadata
end

initializer 'logstasher.load' do
if ::LogStasher.enabled?
config.after_initialize do
if ::LogStasher.enabled? && !::LogStasher.silence_standard_logging?
::ActiveSupport.on_load(:action_controller) do
require 'logstasher/log_subscriber'
require 'logstasher/context_wrapper'
require "logstasher/log_subscriber"
require "logstasher/context_wrapper"

include ::LogStasher::ContextWrapper
end
end
end

config.after_initialize do
if ::LogStasher.enabled? && ::LogStasher.silence_standard_logging?
require 'logstasher/silent_logger'

::Rails::Rack::Logger.send(:include, ::LogStasher::SilentLogger)

::ActiveSupport::LogSubscriber.log_subscribers.each do |subscriber|
if subscriber.is_a?(::ActiveSupport::LogSubscriber)
subscriber.class.send(:include, ::LogStasher::SilentLogger)
end
end
end
end

def default_logger
unless @default_logger
path = ::Rails.root.join('log', "logstash_#{::Rails.env}.log")
::FileUtils.touch(path) # prevent autocreate messages in log

@default_logger = ::Logger.new(path)
end

@default_logger
end
end
end
16 changes: 0 additions & 16 deletions lib/logstasher/silent_logger.rb
View file
Open in desktop

This file was deleted.