Support Yabeda server in a new process #22
Conversation
etsenake
changed the title
|
I could also split this up into multiple PRs if that makes it easier to review unsure who to add as a reviewer though @Envek are you the person to review? 😅 |
There was a problem hiding this comment.
Hey, thank you very much for your pull request! It has many interesting things and many surprising ones :-D
When running in clustered mode if app isn't preloaded the other processes or threads error out with Errno::EADDRINUSE doesn't kill the app just pollutes the start up logs a bit
Trying to run a metrics exporter process from every clustered worker and hope for a luck that some of them will start listening on a port feels very wrong to me. If you want to start separate process, isn't it better to start completely independent process just to export metrics then? Let it be managed separately by your runtime (whether it will be Systemd service on a server or sidecar container in Kubernetes pod).
Explicitly run separate process which will only export metrics from all processes on the same machine and nothing else.
Example of such exporter process can be found here: https://gist.github.com/Envek/96f297c1dfbac8ae5afa7e4abff78f0b
lib/yabeda/prometheus/exporter.rb
Outdated
| def start_server_in_process!(**rack_app_options) | ||
| pid = Process.fork do | ||
| # re-configure yabeda since we're in a new process | ||
| Yabeda.configure! |
There was a problem hiding this comment.
Will it really work?
I believe that if yabeda was already configured before fork, it won't allow to call this method.
See https://github.com/yabeda-rb/yabeda/blob/be2700fb3a3694b4f17c92d6c038ca37221d9ce7/lib/yabeda.rb#L91 (I believe instance variable values are preserved after fork)
There was a problem hiding this comment.
Yeah that’s what I thought too but i was seeing the same behaviour on prefork puma and passenger where if I don’t call configure! again in the process I get an empty page and no metrics are served via the endpoint from what I can tell it won’t serve metrics if they aren’t registered on the specific process. Also if you want, you could pull it down and try it out. Perhaps where I’m installing our metrics is the problem? It’s in an initializer though which seems pretty typical a possible reason could also be because we’re using direct file store? I’ll try out one of the other stores and see if they have the same issue if not I could conditionally call configure!
There was a problem hiding this comment.
Figured out why sub-processes don't have the configured metrics declared, its because the configure!method is called in an after_initialize hook which never gets called in the sub process I'll make sure to check if its already configured like its being done here. If you're forking a process its pretty likely that you're not planning on running your entire app initialization process
|
Also added your yabeda-activejob to the Yabeda plugin list. Thanks for sharing! |
|
Awesome! Apologies for taking so long to get back to this was at a work conference for the past couple days getting those changes in, test some things around calling |
add support for starting the metrics server in a separate process add support for suppressing Errno::EADDRINUSE when running in clustered mode in production apps
etsenake
force-pushed
the
add-starting-metrics-server-in-a-new-process
branch
from
8519389
to
0bd3421
Compare
Should fix #21
I've been working on getting yabeda into our app which runs on passenger. I finally have it running, but faced a couple of issues that I've been able to fix manually in our app but wanted to upstream some of those changes
Issue 1: Thread gets killed by passenger as a long-running request
Issue 2 ✅ : Metrics become pretty bloated when we haven't deployed in a while I imagine this would be a problem on weekends where the app is up for days. The addition of rack deflater shrunk the size of the metrics logged by close to 90% in my testing:
Depending on your scrape timeout on your prometheus server this could or could not be a problem ours is set pretty low but has worked fine for other exporters we have.
Issue 3[Just a minor annoyance 😅 ]: When running in clustered mode if app isn't preloaded the other processes or threads error out with
Errno::EADDRINUSEdoesn't kill the app just pollutes the start up logs a bitChanges
Gearing up to go live Monday with these changes and a couple others others in relation to resque on Monday so any feedback would be greatly appreciated.
Shameless plug: because we use resque I wrote a small extension for metrics around activejob for us to get some insight similar to what y’all have done for sidekiq and faktory. Check it out here