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

fpm: indicate idle/busy in worker process title #5567

Closed
wants to merge 1 commit into from

Conversation

cdanis
Copy link

@cdanis cdanis commented May 13, 2020

On a state change from idle->busy or busy->idle, update the process title
of the affected php-fpm worker process to indicate its status.

On a state change from idle->busy or busy->idle, update the process title
of the affected php-fpm worker process to indicate its status.
@carusogabriel carusogabriel requested a review from bukka May 13, 2020 15:36
@bukka
Copy link
Member

bukka commented May 14, 2020

I'm not really sure if it's a good idea to use proc status for sort of monitoring. It's really not meant to be for that IMHO.

@cdanis
Copy link
Author

cdanis commented May 14, 2020

Setting process title is not an expensive operation, as far as I can tell -- I'm not anticipating a memset and a few strcpys adding too much cost to serving a request. And it provides a very useful at-a-glance indication of how loaded a server is in ps. It also seems useful if you have some long-running / hung requests and want to attach gdb to the right worker.

But I do agree that changing process title this often is not super common, and would understand not accepting the patch.

Out of curiosity, is there any sort of benchmarking suite for php-fpm itself?

@bukka
Copy link
Member

bukka commented May 24, 2020

The status should really be used for those things. I would suggest you create some application that will monitor pool status instead.

Also looking at it, I'm not sure if this actually makes much sense. When one process is in idle state, it doesn't mean that the whole pool is idle so this will just result in confusing values. For example when you have 1 of 5 children idle, then the title could be either busy or idle depending what happened last (0 -> 1 or 2 -> 1).

About the benchmark, there is probably not something specific that we would be using. The way how I usually benchmarked things is running wrk2 on nginx that is in front of fpm...

@bukka
Copy link
Member

bukka commented May 25, 2020

Sorry it would work of course as it's a change of name for each child...

However still think that status should be used instead for such thing.

@cdanis
Copy link
Author

cdanis commented May 25, 2020

Yes, at Wikimedia we run https://github.com/bakins/php-fpm-exporter which scrapes the HTTP status interface and makes it accessible to Prometheus timeseries metrics monitoring.

However, we sometimes have outages where a backend becomes very slow or stops responding entirely (database, another HTTP call, etc). We also set very long request timeouts in PHP, because some of our requests are legitimately very expensive.

The issue is that this leads to situations where 100% of workers become busy, leaving no worker available to respond to the status request itself. This means that during such an incident, we also lose monitoring data about php-fpm itself.

So I started looking into ways to communicate worker status out-of-band from the provided status mechanism, which led to this patch.

@bukka
Copy link
Member

bukka commented May 31, 2020

This is a valid concern and we should find a way how to fix it in a better way. I have been thinking about this and started looking to a different solution and did a bit of preparation for it.

Basically I would like to allow an option for a status to be handled by a different process that doesn't get any other traffic. I'd like to add something like pm.status_listen that would allow serving status through a different port or UDS. Then for example in nginx you could have a different upstream just for the status. What do you think about it?

Internally it would be a new pool with a single child that would share the scoreboard with the main pool. I started looking to the implementation and think it might work unless I'm missing something. It doesn't seem to be that invasive as most of the things are already handled (listening, pm...). It's still a bit of work and there might be some edge cases that might be tricky so can't really say that it will surely work but think it's worth a try as it could be quite useful. Will try to get the patch working and see.

@cdanis
Copy link
Author

cdanis commented Jun 1, 2020

I think a status-only-worker sounds good -- thanks!

@bukka
Copy link
Member

bukka commented Aug 1, 2020

@cdanis just created a PR #5918 implementing pm.status_listen. That should hopefully address your issue so closing this one.

@bukka bukka closed this Aug 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants