WIP: Implement function to get fpm status from php `fastcgi_get_status` by tback · Pull Request #3182 · php/php-src

tback · 2018-03-12T12:54:11Z

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.

dzuelke · 2018-03-12T23:40:28Z

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 posix_getpid can be used for that, the extension may not be enabled) for e.g. logging purposes, otherwise it's a little useless to log certain metrics without being able to correlate events.

tback · 2018-03-13T08:07:07Z

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 fastcgi_get_status()? The latter is implemented already.

getmypid() returns the pid of the current child and is available on all plattforms. Maybe that is sufficient?

dzuelke · 2018-03-13T13:51:47Z

Ah, never mind then, @tback :)

👍

bukka · 2018-03-13T14:18:26Z

sapi/fpm/fpm/fpm_main.c


 static const zend_function_entry cgi_fcgi_sapi_functions[] = {
 	PHP_FE(fastcgi_finish_request,              NULL)
+	PHP_FE(fastcgi_get_status,                  NULL)


I would probably go with fpm_get_status as this more fpm related...

Changed in 8ade2f5

bukka · 2018-03-13T14:22:55Z

sapi/fpm/fpm/fpm_status.c


+/* {{{ proto array fastcgi_get_status
+ * Returns the status of the fastcgi process manager */
+PHP_FUNCTION(fastcgi_get_status)


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);

Changed in 979cec1

bukka · 2018-03-13T14:29:03Z

sapi/fpm/fpm/fpm_status.c

+		if (!scoreboard_p->procs[i] || !scoreboard_p->procs[i]->used) {
+			continue;
+		}
+		proc = *scoreboard_p->procs[i];


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.

meaning it might be a bug in there too... :)

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?

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.

I opened #3185 and implemented safe access in 9eb8db6

motecshine · 2018-03-16T06:35:32Z

sapi/fpm/fpm/fpm_main.c

+{
+	int error = fpm_status_export_to_zval(return_value);
+	if(error){
+		RETURN_FALSE;


I think you should verbose this error

I will do that

bukka · 2018-03-16T19:36:49Z

sapi/fpm/fpm/fpm_main.c

+PHP_FUNCTION(fpm_get_status) /* {{{ */
+{
+	int error = fpm_status_export_to_zval(return_value);
+	if(error){


CS: missing spaces... If you don't need to use error later then this should do:

if (fpm_status_export_to_zval(return_value)) {

motecshine · 2018-03-18T02:52:41Z

sapi/fpm/fpm/fpm_status.c

+	struct fpm_scoreboard_proc_s procs[scoreboard.nprocs];
+
+	struct fpm_scoreboard_proc_s *proc_p;
+	for(i=0; i<scoreboard.nprocs; i++) {


cs: for () {}

carusogabriel · 2018-03-18T15:08:40Z

sapi/fpm/tests/025.phpt

+--TEST--
+FPM: Test fpm_get_status function
+--SKIPIF--
+<?php include "skipif.inc"; ?>


require is preferable, as is safer 🛡️

tback · 2018-03-19T09:00:59Z

This will have to wait until I implemented fpm_copy_scoreboard.

dzuelke · 2018-06-25T20:19:48Z

Is this something we could get into PHP 7.3, together with #2458, @tback and @bukka? Would make a lot of sense IMO to have access to the pool name info for logging in environments where (using that other PR) "... child said" stdout/stderr decoration is disabled.

php-pulls · 2018-07-07T11:55:53Z

Comment on behalf of bukka at php.net:

Merged in 140def4 and test migrated in f86f3ed

Till Backhaus added 2 commits March 12, 2018 13:46

Implement fastcgi_get_status

da30176

Test fastcgi_get_status

00a38ff

tback force-pushed the fastcgi-get-status branch from 9bdbb6f to 00a38ff Compare March 12, 2018 19:26

bukka reviewed Mar 13, 2018

View reviewed changes

Rename fastcgi_get_status to fpm_get_status

8ade2f5

bukka reviewed Mar 13, 2018

View reviewed changes

Split fpm_get_status

979cec1

motecshine reviewed Mar 16, 2018

View reviewed changes

bukka reviewed Mar 16, 2018

View reviewed changes

Copy scoreboard

9eb8db6

motecshine reviewed Mar 18, 2018

View reviewed changes

carusogabriel suggested changes Mar 18, 2018

View reviewed changes

tback changed the title ~~Implement function to get fpm status from php fastcgi_get_status~~ WIP: Implement function to get fpm status from php fastcgi_get_status Mar 19, 2018

tback mentioned this pull request Mar 20, 2018

Implement fpm_scoreboard_copy and use it for fpm_handle_status_request #3188

Closed

Gemorroj mentioned this pull request Apr 10, 2018

Add php info Gemorroj/ginfo#7

Closed

6 tasks

smalyshev added the Feature label Jun 20, 2018

dzuelke mentioned this pull request Jul 4, 2018

FPM logger refactoring #2458

Merged

nikic added the PHP 7.3 Nominated label Jul 7, 2018

php-pulls closed this Jul 7, 2018

hollodotme mentioned this pull request Dec 17, 2018

Fix missing return type of fpm_get_status() function JetBrains/phpstorm-stubs#469

Merged

Conversation

tback commented Mar 12, 2018

Uh oh!

dzuelke commented Mar 12, 2018

Uh oh!

tback commented Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dzuelke commented Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tback commented Mar 19, 2018

Uh oh!

dzuelke commented Jun 25, 2018

Uh oh!

php-pulls commented Jul 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

tback commented Mar 13, 2018 •

edited

Loading

dzuelke commented Mar 13, 2018 •

edited

Loading