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

Fix initialization and make "enabled" setting work #23

Merged

Conversation

haffla
Copy link
Contributor

@haffla haffla commented Mar 8, 2021

Hey!

I noticed that setting enabled to false actually doesn't work as expected. The problem is that when the engine code is executed the initializer hasn't even run yet, so RailsPerformance.enabled is always true.

Fixed it by delaying the middleware configuration using the "rails_performance.middleware" initializer.

Also replaced

  • RailsPerformance.try(:enabled) with just RailsPerformance.enabled (why would the module not respond to enabled?)
  • const_defined? with just defined? (I think it's the correct way to check if a gem is installed and has been required)

@haffla haffla changed the title Fix initialization and make enabled config work Fix initialization and make "enabled" setting work Mar 9, 2021
@haffla
Copy link
Contributor Author

haffla commented Apr 1, 2021

Hello @igorkasyanchuk

Did you get a chance to have a look at this?

@igorkasyanchuk
Copy link
Owner

Hi @haffla ,
sorry not enogh time

So as I see

this option is just disabling access to the admin panel.

And here:

if RailsPerformance.try(:enabled) # for rails c

Have you created an initializer? Are you saying that this option if set to false in initialized later "true" in the code?

@haffla
Copy link
Contributor Author

haffla commented Apr 2, 2021

I am saying that testing for enabled inside of the Engine class (rails_performance/lib/rails_performance/engine.rb) does not work because when you start your Rails app, the Engine class is loaded before any of the Rails initializer files like config/initializers/rails_performance.rb are loaded. Just test it in your own app and set enabled to false. You will see that rails_performance is still collecting metrics because it is actually not disabled.

@igorkasyanchuk
Copy link
Owner

Ohh, I see. I thought I've tested and it was working. Good catch on this. Thanks

@igorkasyanchuk igorkasyanchuk merged commit e347274 into igorkasyanchuk:master Apr 2, 2021
@haffla haffla deleted the feature/fix-initialization branch April 2, 2021 10:39
@igorkasyanchuk
Copy link
Owner

a new version was just released. Thanks for support

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants