Skip to content

Conversation

@m-bucher
Copy link
Contributor

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

@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
@m-bucher m-bucher force-pushed the add_profiling_extensions branch from 1715913 to 2d39e52 Compare November 22, 2023 17:31
ekohl
ekohl previously approved these changes Nov 22, 2023
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
@sbernhard
Copy link
Contributor

can we finish this?

@sbernhard
Copy link
Contributor

do you want to rebase @m-bucher ?

sbernhard pushed a commit to ATIX-AG/foreman that referenced this pull request Feb 9, 2026
Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>

from theforeman#9897
@m-bucher m-bucher force-pushed the add_profiling_extensions branch from 2d39e52 to a550e66 Compare February 10, 2026 13:22
Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
@m-bucher m-bucher force-pushed the add_profiling_extensions branch from a550e66 to 0fb6cb4 Compare February 11, 2026 10:12
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