Refs #28506 - fixed telemetry boot require #7271
Conversation
|
Issues: #28506 |
| def initialize(opts = {}) | ||
| require 'prometheus/client' | ||
| require 'prometheus/client/data_stores/direct_file_store' | ||
| # Set multiprocess-friendly data store | ||
| FileUtils.mkdir_p(PROMETHEUS_STORE_DIR) |
There was a problem hiding this comment.
Instead of a global, should this be store_dir = opts[:store_dir] || File.join(Rails.root, 'tmp', 'prometheus')?
There was a problem hiding this comment.
I don't like introducing new option, then setting, then installer. Let's be opinionated here - these files will rarely be larger than few MBs so Rails cache is an ideal place. You can lost them and it gets regenerated on restart, no big deal too.
There was a problem hiding this comment.
So this is a parameter to the method, which also allows unit testing. I wasn't calling for it to be configurable at the setting/installer level.
There was a problem hiding this comment.
I agree the constant is not necessary as it doesn't need to be this exact dir, it's only a sane default, so why not to have the method more robust. I'm ok with this as well though.
| def initialize(opts = {}) | ||
| require 'prometheus/client' | ||
| require 'prometheus/client/data_stores/direct_file_store' | ||
| # Set multiprocess-friendly data store | ||
| FileUtils.mkdir_p(PROMETHEUS_STORE_DIR) |
There was a problem hiding this comment.
I agree the constant is not necessary as it doesn't need to be this exact dir, it's only a sane default, so why not to have the method more robust. I'm ok with this as well though.
|
Thanks @lzap and everyone. I'll merge this now, if any of you feel like modifying the function feel free to open a follow up PR. |
|
1.24 - e68fc9e |
When bundler group is not active, this would fail.