WIP: Implement function to get fpm status from php fastcgi_get_status
#3182
Conversation
9bdbb6f to
00a38ff
Compare
|
Just one note, and probably trivial to add: I think that for completeness' sake, it should then also be possible to access the current process' FPM pool name and child PID (although |
|
Thanks for the input @dzuelke. Are you suggesting a separate function to return the pool name or do you want the pool name included in the results of
|
|
Ah, never mind then, @tback :) 👍 |
sapi/fpm/fpm/fpm_main.c
Outdated
|
|
||
| static const zend_function_entry cgi_fcgi_sapi_functions[] = { | ||
| PHP_FE(fastcgi_finish_request, NULL) | ||
| PHP_FE(fastcgi_get_status, NULL) |
There was a problem hiding this comment.
I would probably go with fpm_get_status as this more fpm related...
sapi/fpm/fpm/fpm_status.c
Outdated
|
|
||
| /* {{{ proto array fastcgi_get_status | ||
| * Returns the status of the fastcgi process manager */ | ||
| PHP_FUNCTION(fastcgi_get_status) |
There was a problem hiding this comment.
I would prefer to have a function that would just fill the supplied zval and PHP_FUNCTION in fpm_main.c that would call it. It means function like:
int fpm_status_export_to_zval(zval *status);
sapi/fpm/fpm/fpm_status.c
Outdated
| if (!scoreboard_p->procs[i] || !scoreboard_p->procs[i]->used) { | ||
| continue; | ||
| } | ||
| proc = *scoreboard_p->procs[i]; |
There was a problem hiding this comment.
I'm wondering if this is actually safe to do without the lock as technically some value could be changed by other process during this copy. I'm aware that this is also done in fpm_status_handle_request for full syntax but not sure if it's correct in there too.
There was a problem hiding this comment.
meaning it might be a bug in there too... :)
There was a problem hiding this comment.
It struck me too when copy pasting it. I've been reading through the code over the past days.
I assume the worst possible result right now is an inconsistent reading, like number of active processes not being in sync with detailed process stats.
I assume the safe way to do it would be to lock the scoreboard, then iterate through the procs and copy (lock copy unlock) them one by one and only then unlocking the scoreboard.
Is this the path of action you're suggesting?
There was a problem hiding this comment.
Yeah I think that would be safe as we can't just copy the whole scoreboard and procs in one go (each proc can be locked individually..).
It would be good to introduce a new function fpm_scoreboard_copy that would allocate new scoreboard and copy all data into it (first locked scoreboard data and then iterate and copy the procs as you suggested). I think this might make sense to do in separate PR and use it for full syntax. The reason is that this part could be back ported to the stable branches as it seems like a bug to me. This PR can be then based on it.
| { | ||
| int error = fpm_status_export_to_zval(return_value); | ||
| if(error){ | ||
| RETURN_FALSE; |
There was a problem hiding this comment.
I think you should verbose this error
| PHP_FUNCTION(fpm_get_status) /* {{{ */ | ||
| { | ||
| int error = fpm_status_export_to_zval(return_value); | ||
| if(error){ |
There was a problem hiding this comment.
CS: missing spaces... If you don't need to use error later then this should do:
if (fpm_status_export_to_zval(return_value)) {
| struct fpm_scoreboard_proc_s procs[scoreboard.nprocs]; | ||
|
|
||
| struct fpm_scoreboard_proc_s *proc_p; | ||
| for(i=0; i<scoreboard.nprocs; i++) { |
| --TEST-- | ||
| FPM: Test fpm_get_status function | ||
| --SKIPIF-- | ||
| <?php include "skipif.inc"; ?> |
There was a problem hiding this comment.
require is preferable, as is safer 🛡️
fastcgi_get_statusfastcgi_get_status
|
This will have to wait until I implemented fpm_copy_scoreboard. |
fastcgi_get_status allows to get fpm status information from within php. This allows to implement further status pages in php and to integrate fpm metrics with application metrics.