-
Notifications
You must be signed in to change notification settings - Fork 812
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
[php-fpm] new check, get perf insight of your php-fpm install #1441
Conversation
928471a
to
cf0e902
Compare
'accepted conn': 'php_fpm.accepted_conn', | ||
'slow requests': 'php_fpm.slow_requests' | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few suggestions about these names:
- Group some of them (
processes.active
,processes.total
,processes.max_active
, ...) - Make some of them more clear (what are listen_queue and listen_queue_len? Again, group them
listen_queue.something
) - We could call prefix that
fpm.
instead ofphp_fpm
- Use service check names close to what we already have. (
is_connect
is common but maybe not relevant in the case,is_running
sounds better and that's what we have with Gunicorn)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- good idea
- will try that, after re-reading the doc
- I am not sure about this one, the integration is named PHP-FPM, I feel like we should try to keep some consistency in names, or change everything from
php_fpm
tofpm
then but I find it confusing (other stuff is called fpm too...) - I think you meant
can_connect
, in this casecan_connect
is correct in a way, but it's an understatement, because we got a pong from php fpm, meaning the server is ready to process requestsis_running
sound a little bit like a process check to me- it's closer to an "haproxy-like" status heartbeat (people actually use it for that), I am open to suggestions, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The service check name is a good question: the closest integration that we have is Gunicorn, but I am not a big fan of is_running
.
It was just to raise this question and see if we can find something consistent. But since this is very specific, it's better preferring something with more sense. can_ping
?
👍 |
thx for taking this over. Happy to see that this will go forward and eventually be a part of an upcoming release. |
2e1f435
to
d20868a
Compare
@LotharSee fixed your comments. Let me know if the new names are OK and I'll merge it. |
'accepted conn': 'php_fpm.requests.accepted', | ||
'slow requests': 'php_fpm.requests.slow' | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra comments on the names.
First, since I didn't get what the listen_queue
is, I had to look at the doc. This one is pretty good
http://centminmod.com/phpfpm.html#browserstatus
- Try to keep the same depth per "group". For example
php_fpm.processes.active.max
andphp_fpm.processes. max_reached
should have the same number of dots. - Also, finishing by
.max
could be confusing (with histograms)php_fpm.listen_queue_reqs
andphp_fpm.listen_queue_reqs.max
(they also don't have the same depth). Could usemax_
instead. - Not sure what we want to do with "max listen queue", "max active processes" and "max children reached" since they are counters initialized at PHP-FPM startup. Exception with "max children reached": it could be a counter.
d20868a
to
f42db79
Compare
This check targets an install of php-fpm, through the webserver we query the status and ping paths of FPM, and we transform the output of these commands in metrics and service checks. [leo@datadoghq.com] Rebased, fixing up commits together and solved conflicts.
* For metrics and service checks, if there were none reported during the check run, the coverage report would raise because of a division by zero. * Remove the warning message when using count.
* Consolidated in one check ping + status queries * Removed a lot of unused vars, simplified functions, make it look prettier * Set up integration testing on Travis by: * installing a fresh nginx as a fast CGI passthru * installing a PHP version from source which ships the php-fpm binary * making them play together accepting connections to the /status and /ping paths
f42db79
to
a23c7fb
Compare
[php-fpm] new check, get perf insight of your php-fpm install
Yes! I was working on this myself, when is this released to the official deb dd-agent version ? :) |
Hi @jippi . This will go out with our 5.3.0 release, which should be in around 3 weeks! |
In the same time if you want to beta test it, you can just download from github the php-fpm.py file into your /etc/dd-agent/checks.d directory and the yaml file in your /etc/dd-agent/conf.d directory |
@remh shine, going to do just that then :) Thanks |
Changes Unknown when pulling a23c7fb on leo/php_check into * on master*. |
Changelog:
Add first version of check by @arosenhagen
This check targets an install of php-fpm, through the webserver
we query the status and ping paths of FPM, and we transform the
output of these commands in metrics and service checks.
[leo@datadoghq.com] Rebased, fixing up commits together and solved
conflicts.
Revamp the PHP FPM integration
prettier
binary
/status and /ping paths
Supersedes #630
Please review @LotharSee @remh