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

Fixes #36898 - Add rack-mini-profiler for profiling #9897

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

Conversation

m-bucher
Copy link
Contributor

@m-bucher m-bucher commented Nov 7, 2023

bundler.d/profiling.rb Outdated Show resolved Hide resolved
config/initializers/rack_mini_profiler.rb Outdated Show resolved Hide resolved
@m-bucher m-bucher force-pushed the add_profiling_extensions branch from b520fe8 to cab4485 Compare November 10, 2023 14:34
@m-bucher m-bucher force-pushed the add_profiling_extensions branch from cab4485 to 1715913 Compare November 14, 2023 14:52
@m-bucher m-bucher requested a review from ekohl November 16, 2023 13:44
@ekohl ekohl changed the title Add rack-mini-profiler for profiling Fixes #36898 - Add rack-mini-profiler for profiling Nov 20, 2023
Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
@m-bucher m-bucher force-pushed the add_profiling_extensions branch from 1715913 to 2d39e52 Compare November 22, 2023 17:31
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I think this looks good, but I'd like to wait until after branching so we have a bit more time to evaluate it in nightly.

@m-bucher
Copy link
Contributor Author

Agreed, would it make sense to move the bullet-gem into the profiling-group as well?

@ekohl
Copy link
Member

ekohl commented Nov 24, 2023

Possibly, but it's only enabled in development mode:

if defined?(Bullet)
config.after_initialize do
Bullet.enable = true
Bullet.bullet_logger = true
Bullet.console = true
Bullet.rails_logger = true
Bullet.add_footer = true
Bullet.counter_cache_enable = false
end
end

So if you move it to profiling (which you also intend to be usable in production mode) the I'd expect it to setup to also happen in the production environment.

@m-bucher
Copy link
Contributor Author

I tried it, but the bullet.log could not be written in production due to missing permissions :-(

@ekohl
Copy link
Member

ekohl commented Nov 24, 2023

That surprises me because https://github.com/flyerhzm/bullet#configuration states:

Bullet.bullet_logger: log to the Bullet log file (Rails.root/log/bullet.log)

in production Rails.root is /usr/share/foreman and /usr/share/foreman is a symlink to /var/log/foreman, which is owned by foreman. So it should be able to write there.

@sbernhard
Copy link
Contributor

Branching was already done some weeks ago. Can we continue with this?

nadjaheitmann pushed a commit to ATIX-AG/foreman that referenced this pull request Jan 10, 2024
Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>

from theforeman#9897
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.

3 participants