Add openmtrics formatting to fpm status #5723

WyriHaximus · 2020-06-16T07:25:41Z

Todo:

Add tests
Validate Output against Prometheus

bukka · 2020-07-22T17:37:15Z

I have been just thinking about this few days ago and now I see the PR. :)

I support the idea and I'd be happy to merge this in once finished and if I don't see any issue during review.

WyriHaximus · 2020-07-22T18:09:10Z

@bukka Great! Just need to figure out testing, and get it ready for review.

bukka · 2020-07-23T20:36:04Z

You just need to add a format prometheus here:

php-src/sapi/fpm/tests/tester.inc

Line 502 in bc4201f

$formats = ['plain', 'html', 'xml', 'json']

and then create checkStatusPrometheus in status.inc in a similar way like the one for json:

php-src/sapi/fpm/tests/status.inc

Lines 180 to 199 in bc4201f

    
               protected function checkStatusJson(string $body, array $fields) 
        
               { 
        
                   $this->makeStatusCheck( 
        
                       $body, 
        
                       $fields, 
        
                       '"%s":%s,', 
        
                       '{', 
        
                       '}', 
        
                       null, 
        
                       function ($value) { 
        
                           if (is_numeric($value) || $value === '\d+') { 
        
                               return $value; 
        
                           } 
        
                           return '"' . $value . '"'; 
        
                       }, 
        
                       true 
        
                   ); 
        
               } 
        
           }

bukka · 2020-08-01T13:01:19Z

@WyriHaximus would you be able to create a normal PR and squash all your commits. I'd like to merge it before feature freeze which is on 4th Aug. It's in few days so I'd be happy to get it in without test and add test and possibly some tweaks later.

WyriHaximus · 2020-08-01T13:15:44Z

@bukka Done. Was planning to look into the tests tomorrow or Wednesday, but this also works for me. Will look into those anyway whether this PR is merged or not by then.

bukka · 2020-08-01T14:44:40Z

Ok thanks. I plan to merge it on Monday so if you get time tomorrow and manage to add it, then great! Otherwise you can create a new PR after it's in.

WyriHaximus · 2020-08-02T15:20:15Z

@bukka Got a decent stab at the tests, not ensure clear how it works yet but got them up and running. Will continue Wednesday and hope to have some working tests then :).

bukka · 2020-08-03T18:07:59Z

Hmm so I was just looking into this a bit more closely and also testing it. It actually doesn't work. The format syntax doesn't look like format that is supported. At least it doesn't work with the spprintf. Also there are some further issues. That kind of tells me that we shouldn't merge anything until there is a working test. Sorry for trying to speed it up. It will need to wait until it's all ready.

WyriHaximus · 2020-08-03T18:24:44Z

@bukka Would we still be able to get it into 8.0 after the feature freeze?

WyriHaximus · 2020-08-03T18:37:55Z

@bukka Also I completely forgot about this, but the spprintf format is something I forgot about, is there a way to do %2$s like PHP's sprintf?

bukka · 2020-08-03T20:51:38Z

From a quick look to https://github.com/php/php-src/blob/e0fa48f69dd14b52c8f1b2904ac7bd30472849a8/main/spprintf.c I don't think there is such way unless I missed something. Probably better to use a separate call for prometheus format and add pool multiple times.

In terms of FF I would be fine as it is quite self contained but it depends on the release managers if they allow it during beta.

sapi/fpm/fpm/fpm_status.c

bukka · 2020-08-03T20:53:49Z

sapi/fpm/fpm/fpm_status.c

 					proc.request_stage == FPM_REQUEST_ACCEPTING ? cpu : 0.,
-					proc.request_stage == FPM_REQUEST_ACCEPTING ? proc.memory : 0);
+					proc.request_stage == FPM_REQUEST_ACCEPTING ? proc.memory : 0,
+					scoreboard.pool);


this is specific to prometheus so it shouldn't be probably added for all...

Only the current pool will be showed on the status page correct? Because then we can indeed drop it off and it will make things a lot easier

Ok I'll have a look at that, thanks 👍

Sorry commented from the wrong account so needed to remove that account from reviewers.

For reference was saying that it will be fixed by the changes for the spprintf :)

WyriHaximus · 2020-08-03T21:51:26Z

Ok progress, think I have it working (extracting this from the tests):

# HELP phpfpm_up Could pool unconfined using a static PM on PHP-FPM be reached?
# TYPE phpfpm_up gauge
phpfpm_up 1
# HELP phpfpm_start_time When FPM has started.
# TYPE phpfpm_start_time gauge
phpfpm_start_since 140720537860896
# HELP phpfpm_start_since The number of seconds since FPM has started.
# TYPE phpfpm_start_since counter
phpfpm_start_since 0
# HELP phpfpm_accepted_connections The number of requests accepted by the pool.
# TYPE phpfpm_accepted_connections counter
phpfpm_accepted_connections 6
# HELP phpfpm_listen_queue The number of requests in the queue of pending connections.
# TYPE phpfpm_listen_queue gauge
phpfpm_listen_queue 0
# HELP phpfpm_max_listen_queue The maximum number of requests in the queue of pending connections since FPM has started.
# TYPE phpfpm_max_listen_queue counter
phpfpm_max_listen_queue 0
# TYPE phpfpm_listen_queue_length gauge
# HELP phpfpm_listen_queue_length The size of the socket queue of pending connections.
phpfpm_listen_queue_length 128
# HELP phpfpm_idle_processes The number of idle processes.
# TYPE phpfpm_idle_processes gauge
phpfpm_idle_processes 0
# HELP phpfpm_active_processes The number of active processes.
# TYPE phpfpm_active_processes gauge
phpfpm_active_processes 1
# HELP phpfpm_total_processes The number of idle + active processes.
# TYPE phpfpm_total_processes gauge
phpfpm_total_processes 1
# HELP phpfpm_max_active_processes The maximum number of active processes since FPM has started.
# TYPE phpfpm_max_active_processes counter
phpfpm_max_active_processes 1
# HELP phpfpm_max_children_reached The number of times, the process limit has been reached, when pm tries to start more children (works only for pm 'dynamic' and 'ondemand').
# TYPE phpfpm_max_children_reached counter
phpfpm_max_children_reached 0
# HELP phpfpm_slow_requests The number of requests that exceeded your 'request_slowlog_timeout' value.
# TYPE phpfpm_slow_requests counter
phpfpm_slow_requests 0

Looking into fixing those now. And if they work I'll look into getting the pool in there correctly

WyriHaximus · 2020-08-03T23:24:56Z

From a quick look to https://github.com/php/php-src/blob/e0fa48f69dd14b52c8f1b2904ac7bd30472849a8/main/spprintf.c I don't think there is such way unless I missed something. Probably better to use a separate call for prometheus format and add pool multiple times.

Yeah will look into that next, for now I got it into a mostly acceptable shape IMHO. Just spotted one-off indention but will fix that tomorrow morning. Also have some work ready to add the pool, but got some SIGSERV during testing so left that out for now.

In terms of FF I would be fine as it is quite self contained but it depends on the release managers if they allow it during beta.

Lets see and hope @carusogabriel things this is small enough to be allowed to land during beta 🤞

sapi/fpm/fpm/fpm_status.c

WyriHaximus · 2020-08-04T15:54:48Z

@bukka @carusogabriel Right so validated metric using promtool to ensure it's in the correct format. Should have most issues fixed. Two things, I didn't include process metrics because of time constraints. And secondly, I haven't figured out why the tests are failing, the code seems to behave as expected.

bukka · 2020-08-04T17:32:49Z

@WyriHaximus The travis test failure seems related to this PR (status-basic.phpt is failing).

WyriHaximus · 2020-08-04T17:41:26Z

@bukka Yup, there is still something off about the pattern in there. Interestingly enough the CI's don't show the reason. Let me run it locally again and comment the output of the report here

WyriHaximus · 2020-08-04T17:46:45Z

@bukka

================================================================================
/home/wyrihaximus/Projects/PHP/php-src/sapi/fpm/tests/status-basic.phpt
================================================================================
ERROR: Expected body does not match pattern
BODY:
string(2053) "# HELP phpfpm_up Could pool unconfined using a static PM on PHP-FPM be reached?
# TYPE phpfpm_up gauge
phpfpm_up 1
# HELP phpfpm_start_time When FPM has started.
# TYPE phpfpm_start_time gauge
phpfpm_start_time 140724882810112
# HELP phpfpm_start_since_total The number of seconds since FPM has started.
# TYPE phpfpm_start_since_total counter
phpfpm_start_since_total 0
# HELP phpfpm_accepted_connections_total The number of requests accepted by the pool.
# TYPE phpfpm_accepted_connections_total counter
phpfpm_accepted_connections_total 6
# HELP phpfpm_listen_queue The number of requests in the queue of pending connections.
# TYPE phpfpm_listen_queue gauge
phpfpm_listen_queue 0
# HELP phpfpm_max_listen_queue_total The maximum number of requests in the queue of pending connections since FPM has started.
# TYPE phpfpm_max_listen_queue_total counter
phpfpm_max_listen_queue_total 0
# TYPE phpfpm_listen_queue_length gauge
# HELP phpfpm_listen_queue_length The size of the socket queue of pending connections.
phpfpm_listen_queue_length 511
# HELP phpfpm_idle_processes The number of idle processes.
# TYPE phpfpm_idle_processes gauge
phpfpm_idle_processes 0
# HELP phpfpm_active_processes The number of active processes.
# TYPE phpfpm_active_processes gauge
phpfpm_active_processes 1
# HELP phpfpm_total_processes The number of idle + active processes.
# TYPE phpfpm_total_processes gauge
phpfpm_total_processes 1
# HELP phpfpm_max_active_processes_total The maximum number of active processes since FPM has started.
# TYPE phpfpm_max_active_processes_total counter
phpfpm_max_active_processes_total 1
# HELP phpfpm_max_children_reached_total The number of times, the process limit has been reached, when pm tries to start more children (works only for pm 'dynamic' and 'ondemand').
# TYPE phpfpm_max_children_reached_total counter
phpfpm_max_children_reached_total 0
# HELP phpfpm_slow_requests_total The number of requests that exceeded your 'request_slowlog_timeout' value.
# TYPE phpfpm_slow_requests_total counter
phpfpm_slow_requests_total 0"
PATTERN:
string(2047) "|# HELP phpfpm_up Could pool unconfined using a static PM on PHP-FPM be reached?
# TYPE phpfpm_up gauge
phpfpm_up 1
# HELP phpfpm_start_time When FPM has started.
# TYPE phpfpm_start_time gauge
phpfpm_start_time \d+
# HELP phpfpm_start_since_total The number of seconds since FPM has started.
# TYPE phpfpm_start_since_total counter
phpfpm_start_since_total \d+
# HELP phpfpm_accepted_connections_total The number of requests accepted by the pool.
# TYPE phpfpm_accepted_connections_total counter
phpfpm_accepted_connections_total \d+
# HELP phpfpm_listen_queue The number of requests in the queue of pending connections.
# TYPE phpfpm_listen_queue gauge
phpfpm_listen_queue 0
# HELP phpfpm_max_listen_queue_total The maximum number of requests in the queue of pending connections since FPM has started.
# TYPE phpfpm_max_listen_queue_total counter
phpfpm_max_listen_queue_total 0
# TYPE phpfpm_listen_queue_length gauge
# HELP phpfpm_listen_queue_length The size of the socket queue of pending connections.
phpfpm_listen_queue_length \d+
# HELP phpfpm_idle_processes The number of idle processes.
# TYPE phpfpm_idle_processes gauge
phpfpm_idle_processes 0
# HELP phpfpm_active_processes The number of active processes.
# TYPE phpfpm_active_processes gauge
phpfpm_active_processes 1
# HELP phpfpm_total_processes The number of idle + active processes.
# TYPE phpfpm_total_processes gauge
phpfpm_total_processes 1
# HELP phpfpm_max_active_processes_total The maximum number of active processes since FPM has started.
# TYPE phpfpm_max_active_processes_total counter
phpfpm_max_active_processes_total 1
# HELP phpfpm_max_children_reached_total The number of times, the process limit has been reached, when pm tries to start more children (works only for pm 'dynamic' and 'ondemand').
# TYPE phpfpm_max_children_reached_total counter
phpfpm_max_children_reached_total 0
# HELP phpfpm_slow_requests_total The number of requests that exceeded your 'request_slowlog_timeout' value.
# TYPE phpfpm_slow_requests_total counter
phpfpm_slow_requests_total 0|"
Done
================================================================================
001+ ERROR: Expected body does not match pattern
001- Done
002+ BODY:
003+ string(2053) "# HELP phpfpm_up Could pool unconfined using a static PM on PHP-FPM be reached?
004+ # TYPE phpfpm_up gauge
005+ phpfpm_up 1
006+ # HELP phpfpm_start_time When FPM has started.
007+ # TYPE phpfpm_start_time gauge
008+ phpfpm_start_time 140724882810112
009+ # HELP phpfpm_start_since_total The number of seconds since FPM has started.
010+ # TYPE phpfpm_start_since_total counter
011+ phpfpm_start_since_total 0
012+ # HELP phpfpm_accepted_connections_total The number of requests accepted by the pool.
013+ # TYPE phpfpm_accepted_connections_total counter
014+ phpfpm_accepted_connections_total 6
015+ # HELP phpfpm_listen_queue The number of requests in the queue of pending connections.
016+ # TYPE phpfpm_listen_queue gauge
017+ phpfpm_listen_queue 0
018+ # HELP phpfpm_max_listen_queue_total The maximum number of requests in the queue of pending connections since FPM has started.
019+ # TYPE phpfpm_max_listen_queue_total counter
020+ phpfpm_max_listen_queue_total 0
021+ # TYPE phpfpm_listen_queue_length gauge
022+ # HELP phpfpm_listen_queue_length The size of the socket queue of pending connections.
023+ phpfpm_listen_queue_length 511
024+ # HELP phpfpm_idle_processes The number of idle processes.
025+ # TYPE phpfpm_idle_processes gauge
026+ phpfpm_idle_processes 0
027+ # HELP phpfpm_active_processes The number of active processes.
028+ # TYPE phpfpm_active_processes gauge
029+ phpfpm_active_processes 1
030+ # HELP phpfpm_total_processes The number of idle + active processes.
031+ # TYPE phpfpm_total_processes gauge
032+ phpfpm_total_processes 1
033+ # HELP phpfpm_max_active_processes_total The maximum number of active processes since FPM has started.
034+ # TYPE phpfpm_max_active_processes_total counter
035+ phpfpm_max_active_processes_total 1
036+ # HELP phpfpm_max_children_reached_total The number of times, the process limit has been reached, when pm tries to start more children (works only for pm 'dynamic' and 'ondemand').
037+ # TYPE phpfpm_max_children_reached_total counter
038+ phpfpm_max_children_reached_total 0
039+ # HELP phpfpm_slow_requests_total The number of requests that exceeded your 'request_slowlog_timeout' value.
040+ # TYPE phpfpm_slow_requests_total counter
041+ phpfpm_slow_requests_total 0"
042+ PATTERN:
043+ string(2047) "|# HELP phpfpm_up Could pool unconfined using a static PM on PHP-FPM be reached?
044+ # TYPE phpfpm_up gauge
045+ phpfpm_up 1
046+ # HELP phpfpm_start_time When FPM has started.
047+ # TYPE phpfpm_start_time gauge
048+ phpfpm_start_time \d+
049+ # HELP phpfpm_start_since_total The number of seconds since FPM has started.
050+ # TYPE phpfpm_start_since_total counter
051+ phpfpm_start_since_total \d+
052+ # HELP phpfpm_accepted_connections_total The number of requests accepted by the pool.
053+ # TYPE phpfpm_accepted_connections_total counter
054+ phpfpm_accepted_connections_total \d+
055+ # HELP phpfpm_listen_queue The number of requests in the queue of pending connections.
056+ # TYPE phpfpm_listen_queue gauge
057+ phpfpm_listen_queue 0
058+ # HELP phpfpm_max_listen_queue_total The maximum number of requests in the queue of pending connections since FPM has started.
059+ # TYPE phpfpm_max_listen_queue_total counter
060+ phpfpm_max_listen_queue_total 0
061+ # TYPE phpfpm_listen_queue_length gauge
062+ # HELP phpfpm_listen_queue_length The size of the socket queue of pending connections.
063+ phpfpm_listen_queue_length \d+
064+ # HELP phpfpm_idle_processes The number of idle processes.
065+ # TYPE phpfpm_idle_processes gauge
066+ phpfpm_idle_processes 0
067+ # HELP phpfpm_active_processes The number of active processes.
068+ # TYPE phpfpm_active_processes gauge
069+ phpfpm_active_processes 1
070+ # HELP phpfpm_total_processes The number of idle + active processes.
071+ # TYPE phpfpm_total_processes gauge
072+ phpfpm_total_processes 1
073+ # HELP phpfpm_max_active_processes_total The maximum number of active processes since FPM has started.
074+ # TYPE phpfpm_max_active_processes_total counter
075+ phpfpm_max_active_processes_total 1
076+ # HELP phpfpm_max_children_reached_total The number of times, the process limit has been reached, when pm tries to start more children (works only for pm 'dynamic' and 'ondemand').
077+ # TYPE phpfpm_max_children_reached_total counter
078+ phpfpm_max_children_reached_total 0
079+ # HELP phpfpm_slow_requests_total The number of requests that exceeded your 'request_slowlog_timeout' value.
080+ # TYPE phpfpm_slow_requests_total counter
081+ phpfpm_slow_requests_total 0|"
082+ Done
================================================================================

JulienBreux · 2020-10-27T08:50:45Z

Hi guys!

Thanks a lot for this amazing missing feature!

How can I help you?

Have a good day 🚀

WyriHaximus · 2020-10-27T12:38:06Z

Hey @JulienBreux so the thing that this PR is currently stuck at is failing tests. So if you have insights how to get them to pass that would be great. Also since it didn't make the PHP 8.0 feature freeze it will take until PHP 8.1 to get in (so give or take a year), I wasn't in a hurry to get it finished after missing that deadline.

sapi/fpm/tests/status.inc

sapi/fpm/fpm/fpm_status.c

sapi/fpm/tests/status.inc

sapi/fpm/tests/tester.inc

lcobucci

Silly me, escaped the wrong thing

sapi/fpm/fpm/fpm_status.c

sapi/fpm/tests/status.inc

WyriHaximus · 2021-01-05T21:52:44Z

Thanks for the help getting the tests fixed @lcobucci 👍 !

lcobucci · 2021-01-05T22:08:48Z

Thanks for the help getting the tests fixed @lcobucci !

My pleasure, @WyriHaximus. This is a great idea and will help us to instrument PHP infra in a much simpler way.

sapi/fpm/tests/status.inc

mfn · 2021-03-12T20:56:14Z

Everything is green? Can we merge? :)

WyriHaximus · 2021-03-12T21:10:24Z

@mfn Think so, but want to do one more review of it this weekend before I'm fully comfortable with it :D

WyriHaximus · 2021-03-14T18:34:32Z

@mfn @bukka As far as I'm concerned this PR is ready to be merged:

bukka · 2021-03-21T18:34:31Z

Merged via d36ac96 . Thanks!

nikic · 2021-03-22T11:48:09Z

status-basic.phpt and status-listen.phpt fail on MACOS_DEBUG_ZTS and DEBUG_ZTS_MSAN: https://dev.azure.com/phpazuredevops/PHP/_build/results?buildId=15461&view=ms.vss-test-web.build-test-results-tab&runId=353212&paneView=debug&resultId=115352

bukka · 2021-03-22T21:36:19Z

@nikic Thanks for the report. Looks like phpfpm_listen_queue was using incorrect format type. When checking that I also noticed that there is a mistake with the start time so fixed that too in 72a22d9 .

bukka · 2021-03-22T21:45:13Z

@WyriHaximus I'm wondering if there's much point to have the timestamp at all. As I needed to create another conditional branch for that, think we could just omit it completely as I'm not sure if it's useful. I was looking to https://github.com/hipages/php-fpm_exporter#metrics-collected and it has got just phpfpm_start_since so maybe we could use just that?

I'm also wondering if it would make sense to sync the metric names between this exporter (as it's probably the most popular one that I'm aware of) and what's in FPM now. That could make migration easier possibly. What do you think?

One more thing that came to my mind. Would it make sense to add a pool name as a label. I remember that you had it implemented initially, right?

WyriHaximus · 2021-03-23T12:05:32Z

@WyriHaximus I'm wondering if there's much point to have the timestamp at all. As I needed to create another conditional branch for that, think we could just omit it completely as I'm not sure if it's useful. I was looking to https://github.com/hipages/php-fpm_exporter#metrics-collected and it has got just phpfpm_start_since so maybe we could use just that?

Fine by me 👍 .

I'm also wondering if it would make sense to sync the metric names between this exporter (as it's probably the most popular one that I'm aware of) and what's in FPM now. That could make migration easier possibly. What do you think?

Agreed 👍 . Also what are your thoughts on https://github.com/hipages/php-fpm_exporter#why---phpfpmfix-process-count ?

One more thing that came to my mind. Would it make sense to add a pool name as a label. I remember that you had it implemented initially, right?

Yes, I also included the code for support full that provides a per process set of metrics, including the pool. Still up for adding that, but starting with just the basic metrics is a good first step.

bukka · 2021-07-20T22:10:41Z

@WyriHaximus PR created: #7291 to sync the metrics names and remove start timestamp.

Also what are your thoughts on https://github.com/hipages/php-fpm_exporter#why---phpfpmfix-process-count ?

This is really a bug that should be addressed. Will add it on my TODO list :)

WyriHaximus · 2021-07-20T22:18:40Z

@bukka Sweet, let me have a look!

carusogabriel added the SAPI: fpm label Jun 16, 2020

WyriHaximus force-pushed the add-prometheus-formatting-to-fpm-status branch from d561130 to bd87d82 Compare August 1, 2020 13:14

WyriHaximus marked this pull request as ready for review August 1, 2020 13:15

WyriHaximus changed the title ~~[WIP] Add prometheus formatting to fpm status~~ Add prometheus formatting to fpm status Aug 1, 2020

nikic added this to the PHP 8.0 milestone Aug 3, 2020

bukka reviewed Aug 3, 2020

View reviewed changes

sapi/fpm/fpm/fpm_status.c Outdated Show resolved Hide resolved

bukka reviewed Aug 3, 2020

View reviewed changes

sapi/fpm/fpm/fpm_status.c Outdated Show resolved Hide resolved

bukka reviewed Aug 3, 2020

View reviewed changes

WyriHaximus force-pushed the add-prometheus-formatting-to-fpm-status branch 2 times, most recently from 642e4fd to aa57253 Compare August 3, 2020 23:11

carusogabriel reviewed Aug 4, 2020

View reviewed changes

sapi/fpm/fpm/fpm_status.c Outdated Show resolved Hide resolved

WyriHaximus force-pushed the add-prometheus-formatting-to-fpm-status branch from 39bf17f to 9e69627 Compare August 4, 2020 15:52

carusogabriel removed this from the PHP 8.0 milestone Aug 12, 2020