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

feat(php_fpm): track maximum active processes #16193

Merged

Conversation

tcarrio
Copy link
Contributor

@tcarrio tcarrio commented Nov 10, 2023

What does this PR do?

The PHP-FPM status page also provides the "max active processes" statistic, which translates to the maximum number of processes that can be executed at once. This is particularly useful in understanding saturation within the PHP-FPM container, in a manner that allows horizontal scaling in a more intelligent way than the built-in CPU and memory metrics in Kubernetes. This pairs well with the Datadog External Metrics functionality as well

Motivation

We turned on the External Metrics functionality with Datadog. I wanted to implement saturation-based scaling signals that the Datadog Agent provides through the various php_fpm.* metrics, but unfortunately there isn't a metric yet for detecting the maximum number of workers yet. So, here it is!

Additional Notes

I've been going off of the status pages for our internal PHP-FPM pods, an example of the /status output:

{
  "pool": "www",
  "process manager": "dynamic",
  "start time": 1699639472,
  "start since": 8056,
  "accepted conn": 3006,
  "listen queue": 0,
  "max listen queue": 0,
  "listen queue len": 511,
  "idle processes": 4,
  "active processes": 1,
  "total processes": 5,
  "max active processes": 9,
  "max children reached": 0,
  "slow requests": 0
}

I went with php_fpm.processes.max but we could change this to php_fpm.processes.max_active to tie it closer to the actual name.

Lastly, a support ticket was opened for this feature request

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Changelog entries must be created for modifications to shipped code
  • Add the qa/skip-qa label if the PR doesn't need to be tested during QA.

@ghost ghost added the integration/php_fpm label Nov 10, 2023
The PHP-FPM status page also provides the "max active processes" statistic, which translates to the maximum number
of processes that can be executed at once. In the PHP-FPM configuration, this is controlled by the pm.max_children
key. This is particularly useful in understanding saturation within the PHP-FPM container, in a manner that allows
horizontal scaling in a more intelligent way than the built-in CPU and memory metrics in Kubernetes. This pairs
well with the Datadog External Metrics functionality as well
@tcarrio tcarrio force-pushed the feat/php-fpm-track-max-active-processes branch from f1f538a to 1fd72da Compare November 10, 2023 20:26
Copy link

github-actions bot commented Nov 10, 2023

Test Results

  4 files    4 suites   47s ⏱️
17 tests 17 ✔️ 0 💤 0
38 runs  34 ✔️ 4 💤 0

Results for commit b30c94e.

♻️ This comment has been updated with latest results.

Copy link

codecov bot commented Nov 10, 2023

Codecov Report

Merging #16193 (b30c94e) into master (9673f84) will increase coverage by 0.21%.
Report is 38 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files
Flag Coverage Δ
activemq ?
cassandra ?
hive ?
hivemq ?
jboss_wildfly ?
kafka ?
php_fpm 90.53% <ø> (+0.82%) ⬆️
presto ?
solr ?
tomcat ?

Flags with carried forward coverage won't be shown. Click here to find out more.

@tcarrio tcarrio marked this pull request as ready for review November 10, 2023 20:58
@tcarrio tcarrio requested a review from a team as a code owner November 10, 2023 20:58
Copy link
Member

@FlorentClarret FlorentClarret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey 👋

Looks good to me! Would you mind adding a new line to the metadata.csv? Once done, it should be ready to merge.

Copy link
Member

@FlorentClarret FlorentClarret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm gonna merge this one and update the file, no worries 🙂

Thanks again for your contribution!

@FlorentClarret FlorentClarret merged commit 344fdf5 into DataDog:master Nov 24, 2023
35 checks passed
@tcarrio
Copy link
Contributor Author

tcarrio commented Nov 26, 2023

Thank you @FlorentClarret 🙏

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.

2 participants