WIP: Overhaul some behaviors of this gem #13

film42 · 2022-01-31T04:14:08Z

You should be able to set enabled=false and that means you will not
log anything. Period.
You should be able to set enabled=true with
silence_standard_logging=true and it will not include any of the
log subscriber code, but it will allow you to log anything custom.

You should be able to recreate existing behavior with a different
configuration. Example:

# directly updating code
::LogStasher.logger = ::Logger.new(
  ::Rails.root.join("log", "logstash_#{Rails.env}.log"))

# via the device factory
device = ::LogStasher::Device::Factory(type: :file, path:
  ::Rails.root.join("log", "logstash_#{Rails.env}.log"))
::LogStasher.logger = ::Logger.new(device)

NOTE: This is a breaking change, but it might make default behaviors
easier to understand. Right now when enable=false it causes logs to be
piped to the logstash log file for rails apps, and that seems very
awkward. Maybe this is worth fixing?

1. You should be able to set `enabled=false` and that means you will not log anything. Period. 2. You should be able to set `enabled=true` with `silence_standard_logging=true` and it will not include any of the log subscriber code, but it will allow you to log anything custom. 3. You should be able to recreate existing behavior with a different configuration. Example: ``` # directly updating code ::LogStasher.logger = ::Logger.new( ::Rails.root.join("log", "logstash_#{Rails.env}.log")) # via the device factory device = ::LogStasher::Device::Factory(type: :file, path: ::Rails.root.join("log", "logstash_#{Rails.env}.log")) ::LogStasher.logger = ::Logger.new(device) ``` NOTE: This is a breaking change, but it might make default behaviors easier to understand. Right now when `enable=false` it causes logs to be piped to the logstash log file for rails apps, and that seems very awkward. Maybe this is worth fixing?

sshock · 2022-01-31T04:22:13Z

lib/logstasher.rb

@@ -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?


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

sshock · 2022-01-31T04:28:07Z

lib/logstasher.rb


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


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

sshock · 2022-01-31T04:39:31Z

lib/logstasher.rb

@@ -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?
+
      @logger ||= initialize_logger


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.

sshock · 2022-01-31T04:43:08Z

lib/logstasher/railtie.rb

@@ -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


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

The config/environments/*.rb file in the consuming project.

The config/initializers/logstasher.rb in the consuming project.

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.)

quixoten · 2022-01-31T05:13:41Z

`silence_standard_logging` is not meant to disable what LogStasher logs, i.e., logs going to `logstasher_production.log`, it's meant to disable the "standard" logs, i.e., logs going to `priduction.log`. This lets you avoid duplicating logs when you're using LogStasher. I think you want a new option, something like `subscribe_to_rails_events = false`, which defaults to `true`. `enabled = false` is meant to disable the gem entirely. This should be functionally equivalent to removing the LogStasher gem from the project. No other options should have any effect when `enabled = false`. If it's still logging to `logstasher_production.log` in this case, that is definitely a bug.

…

On Sun, Jan 30, 2022, 9:14 PM Garrett T ***@***.***> wrote: 1. You should be able to set enabled=false and that means you will not log anything. Period. 2. You should be able to set enabled=true with silence_standard_logging=true and it will not include any of the log subscriber code, but it will allow you to log anything custom. 3. You should be able to recreate existing behavior with a different configuration. Example: # directly updating code ::LogStasher.logger = ::Logger.new( ::Rails.root.join("log", "logstash_#{Rails.env}.log")) # via the device factory device = ::LogStasher::Device::Factory(type: :file, path: ::Rails.root.join("log", "logstash_#{Rails.env}.log")) ::LogStasher.logger = ::Logger.new(device) NOTE: This is a breaking change, but it might make default behaviors easier to understand. Right now when enable=false it causes logs to be piped to the logstash log file for rails apps, and that seems very awkward. Maybe this is worth fixing? ------------------------------ You can view, comment on, or merge this pull request online at: #13 Commit Summary - e28562c <e28562c> WIP: Overhaul some behaviors of this gem File Changes (3 files <https://github.com/mxenabled/logstasher/pull/13/files>) - *M* lib/logstasher.rb <https://github.com/mxenabled/logstasher/pull/13/files#diff-6727471db7301f4e0affb983705b1599965562437e4107329c494bf823c8f1ca> (11) - *M* lib/logstasher/device.rb <https://github.com/mxenabled/logstasher/pull/13/files#diff-22beed01941a29d009d35371da524ca21f6b2c3c00e19d066c21ee56d66e43e5> (2) - *M* lib/logstasher/railtie.rb <https://github.com/mxenabled/logstasher/pull/13/files#diff-aab033f0706623f2f4823e78b77411b43e9f9ed01c630df6979e97bbca14f4b1> (35) Patch Links: - https://github.com/mxenabled/logstasher/pull/13.patch - https://github.com/mxenabled/logstasher/pull/13.diff — Reply to this email directly, view it on GitHub <#13>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAPRO4KCOEJ7IRIOSCCOADUYYEBXANCNFSM5NFG3WVA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

quixoten · 2022-01-31T18:55:54Z

I need to make an addendum to my previous comment (dusting off some old memories here). I think what I said about silence_standard_logging holds true. It is meant to disable the default rails logging, and not meant to act as a switch for what LogStasher itself logs; however, enabled is a bit trickier, since LogStasher has been co-opted to be more than just a rails request logger. In the current (before this change) implementation, LogStasher effectively supports three "modes" (even though there are four permutations of enabled and silence_standard_logging):

enabled setting	silence_standard_logging setting	resulting mode
true	false	log rails requests through logstasher and default rails logging
true	true	log rails requests through logstasher only
false	false	log rails requests through default rails logging only
false	true	log rails requests through default rails logging only

Neither of these settings modify what LogStasher does when you write to it directly, which I suspect, was a use-case outside of the scope of the original implementation. Perhaps this confusion could be avoided by aliasing enabled as log_rails_requests_through_logstasher, which more clearly reflects the scope of what the setting effects. There's probably a clearer name for silence_standard_logging as well. Alternatively, maybe both of these settings just need to be clearly scoped to the railtie, i.e., railtie_enabled, railtie_silence_standard_logging, since that's all they're intended to modify.

film42 force-pushed the gt/rebuild branch from e28562c to 58a6aa4 Compare January 31, 2022 04:15

sshock reviewed Jan 31, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Overhaul some behaviors of this gem #13

WIP: Overhaul some behaviors of this gem #13

film42 commented Jan 31, 2022

sshock Jan 31, 2022 •

edited

Loading

sshock Jan 31, 2022

sshock Jan 31, 2022

sshock Jan 31, 2022 •

edited

Loading

quixoten commented Jan 31, 2022 via email

quixoten commented Jan 31, 2022

WIP: Overhaul some behaviors of this gem #13

Are you sure you want to change the base?

WIP: Overhaul some behaviors of this gem #13

Conversation

film42 commented Jan 31, 2022

sshock Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

sshock Jan 31, 2022

Choose a reason for hiding this comment

sshock Jan 31, 2022

Choose a reason for hiding this comment

sshock Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

quixoten commented Jan 31, 2022 via email

quixoten commented Jan 31, 2022

sshock Jan 31, 2022 •

edited

Loading

sshock Jan 31, 2022 •

edited

Loading