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.

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

Avoid overriding previously defined custom options for lograge #750

Conversation

bolshakov
Copy link

Prevent overriding lograge.custom_options defined in main app.

Copy link
Contributor

@nilbus nilbus left a comment

Choose a reason for hiding this comment

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

This looks good to me. Seems like a good fix for #749.

I like that it properly defers evaluation of the previous_lograge_custom_options lambda until the replacement config.lograge.custom_options lambda is evaluated.

@halilim
Copy link
Contributor

halilim commented Jan 17, 2018

One thing to note: custom_options can be a Hash.

I think a better option would be Lograge supporting an array/custom object that supports appending/prepending to it (edit: I've created roidrage/lograge#236 to track the suggestion).

config.lograge.custom_options = lambda do |event|
{ es: event.payload[:elasticsearch_runtime].to_f.round(2) }
previous_lograge_custom_options_hash = if previous_lograge_custom_options
previous_lograge_custom_options.call(event)
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch @halilim. This assumes that previous_lograge_custom_options responds to call, which it won't when it's a Hash.

@bolshakov bolshakov force-pushed the feature/lograge-custom-options-override branch 2 times, most recently from 58c4c18 to cfb6b69 Compare January 18, 2018 08:27
@bolshakov
Copy link
Author

I'm absolutely agree that having multiple custom_options is more flexible. Currently I have the following fix in my application.rb and it works an applications side

  class Application < Rails::Application
    # Load it after ELC's lograge engine to fix overriding custom_options
    # @see https://github.com/elastic/elasticsearch-rails/issues/749
    initializer 'custom_lograge', after: 'elasticsearch.lograge' do
      previous_lograge_custom_options = config.lograge.custom_options
      config.lograge.custom_options = lambda do |event|
        previous_lograge_custom_options.call(event).merge(
          user: event.payload[:user],
        )
      end
    end
  end

I updated this PR as well to support hash in custom_options.

@bolshakov bolshakov force-pushed the feature/lograge-custom-options-override branch from cfb6b69 to b8c9b11 Compare August 4, 2018 13:01
@leiz-me
Copy link

leiz-me commented Aug 11, 2018

When will this PR get into the master or any release version that may update with bundler?

@bolshakov
Copy link
Author

@nilbus I tried to sign CLA, but it says browser's cookies are disabled (this is not true), so the form doesn't work for me.

@estolfo
Copy link
Contributor

estolfo commented Jul 12, 2019

@bolshakov can you try signing the CLA again please? Thanks

@bolshakov
Copy link
Author

Unfortunately it still says I have disabled cookies.

@estolfo
Copy link
Contributor

estolfo commented Jul 12, 2019

Would you mind trying in a different browser?

@bolshakov
Copy link
Author

I managed to open the agreement and unfortunately can't assess this legal document and possible consequences for me. So, I'm closing this PR.

@bolshakov bolshakov closed this Jul 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants