Add support for full php-fpm status (#7529) #8394

narph · 2018-09-21T16:46:43Z

I have exposed a new config variable status_full: true which optionally retrieves a more detailed status output from php-fpm (/status?full).

This fixes #7529

Description on process details can be found here https://brandonwamboldt.ca/understanding-the-php-fpm-status-page-1603/

I have extracted the set query parameter handling to a separate func SetQueryParams so I can reuse the functionality inside the Fetch method (pool.go) as the url parser parse.URLHostParserBuilder was not flexible enough to read to read from the config.

ruflin · 2018-09-24T12:58:42Z

metricbeat/module/php_fpm/pool/data.go

-			"max_listen_queue": c.Int("max listen queue"),
-			"queued":           c.Int("listen queue"),
+type phpFpmStatus struct {
+	Name               string          `json:"pool"`


What is the reason you switched from the schema library to defining the types?

hi @ruflin , the response returned an array of objects and could not find a way to preserve it using the schema lib. Also remapped everything in the baseEvent (https://github.com/elastic/beats/pull/8394/files#diff-6fa6396eb9530b802ab7b56fb9a8f13fR88) in order to replace the spaces with underscore.
Let me know if there is a better way to handle this.

ruflin · 2018-09-24T12:59:59Z

Hi @narph Thanks for the contribution. Haven't checked the PR in detail yet but can you add a changelog entry and run make update? This should bring all the generated files up-to-date.

jsoriano

@narph awesome contribution, thanks! I wonder if it would be better to do it as a different metricset istead of an optional extension of an existing one. Let me know what you think.

jsoriano · 2018-09-25T09:32:06Z

metricbeat/module/php_fpm/pool/data.go

+			"script":              process.Script,
+			"last_request_cpu":    process.LastRequestCPU,
+			"last_request_memory": process.LastRequestMemory,
+		}


I think this would be more valuable as an event for each process, in a similar fashion to system process and process_summary metricsets.
We could keep the current metricset as a summary of current processes and connections, and create a new one with the info for each process in a different event.
What do you think?

metricbeat/mb/parse/url.go

metricbeat/module/php_fpm/url.go

jsoriano

Thanks for changing it to a new metricset, this is looking great, I have added some comments.

jsoriano · 2018-09-26T08:51:31Z

metricbeat/module/php_fpm/process/data.go

+	//remapping process details to match the naming format
+	for _, process := range status.Processes {
+		proc := common.MapStr{
+			"pool_name":           status.Name,


This could be placed as php_fpm.pool.name using the ModuleFields attribute of mb.Event. This way it uses a common schema for common fields in both metricsets.

good suggestion, I have addressed it in the latest commit

jsoriano · 2018-09-26T08:54:59Z

metricbeat/module/php_fpm/process/process.go

+// MetricSet has been created then Fetch will begin to be called periodically.
+func init() {
+	mb.Registry.MustAddMetricSet("php_fpm", "process", New, mb.WithHostParser(php_fpm.HostParser),
+		mb.DefaultMetricSet())


Not sure if this metricset should be a default one. In the previous approach, was the full mode intended to be enabled by default?

copy/paste error, learned something new today, pool remains the default metricset

jsoriano · 2018-09-26T08:56:58Z

metricbeat/module/php_fpm/url.go

+	DefaultPath:   defaultPath,
+	QueryParams:   defaultQueryParams,
+	PathConfigKey: "status_path",
+}.Build()


Indentation here looks weird.

metricbeat/module/php_fpm/pool/pool.go

metricbeat/module/php_fpm/process/process.go

jsoriano · 2018-09-26T09:03:59Z

CHANGELOG.asciidoc

@@ -137,6 +137,7 @@ https://github.com/elastic/beats/compare/v6.4.0...master[Check the HEAD diff]
 - Added 'died' PID state to process_system metricset on system module{pull}8275[8275]
 - Added `ccr` metricset to Elasticsearch module. {pull}8335[8335]
 - Support for Kafka 2.0.0 {pull}8399[8399]
+- Add support for `full` status page output for php-fpm module {pull}8394[8394]


We may want to mention here that a new metricset has been added.

jsoriano

It mostly LGTM, just a small comment about reference config.

jsoriano · 2018-09-27T14:32:05Z

metricbeat/module/php_fpm/_meta/config.reference.yml

@@ -1,5 +1,5 @@
 - module: php_fpm
-  metricsets: ["pool"]
+  metricsets: ["pool", "process"]


This could be written as a list for reference with non-default metricset commented out, like this:

metricsets: - pool #- process

hi @jsoriano , I have made the changes in my latest commit.
I have tried using #8292 in order to clean up the handling of the "full" query param (inside the Fetch func) but it seems that I need the "query" config setting to reside somewhere at the metricset level and not at module level as it is at the moment. Am I missing something obvious here?

Details on proceses and description here https://brandonwamboldt.ca/understanding-the-php-fpm-status-page-1603/

Now adhering to goimports

UnicodeEncodeError: 'ascii' codec can't encode character u'\u2026' in position 41: ordinal not in range(128)

Using make on WSL (Windows 10 + Ubuntu)

This emits separate events for each process in the pool "processes" list.

- add `pool.name` using the ModuleFields attribute of mb.Event, updated fields.yml at module level - changed the process metricset to non-default (`pool` seems to be the default metricset in this case) - fixed formatting for url.go - fix and tested integration tests for `pool` and `process` metricsets after updating Fetch method (ReporterV2) - generated headers using `make add-headers` - updated the changelog

@jsoriano

thanks @jsoriano for elastic#8466

jsoriano · 2018-09-28T11:33:01Z

jenkins, test this again please

Add new `process` metricset to collect list of processes in php fpm pools using the full status report. (cherry picked from commit 184793f)

narph requested a review from ruflin September 24, 2018 11:22

ruflin reviewed Sep 24, 2018

View reviewed changes

ruflin added module review Metricbeat Metricbeat :Generator Related to code generators for building custom Beats or modules. labels Sep 24, 2018

narph force-pushed the php-fpm-full branch from ac7de13 to 6b9a561 Compare September 24, 2018 19:42

ruflin requested a review from jsoriano September 25, 2018 07:30

jsoriano reviewed Sep 25, 2018

View reviewed changes

narph force-pushed the php-fpm-full branch from 6b9a561 to add897f Compare September 25, 2018 17:49

houndci-bot reviewed Sep 25, 2018

View reviewed changes

metricbeat/module/php_fpm/url.go Show resolved Hide resolved

jsoriano requested changes Sep 26, 2018

View reviewed changes

This was referenced Sep 27, 2018

Remove line carriages on asset generation #8464

Merged

Cherry-pick #8464 to 6.x: Remove line carriages on asset generation #8465

Merged

Cherry-pick #8464 to 6.4: Remove line carriages on asset generation #8466

Merged

jsoriano approved these changes Sep 27, 2018

View reviewed changes

narph added 9 commits September 27, 2018 16:38

Add support for full php-fpm status (elastic#7529)

f8b1e3e

Details on proceses and description here https://brandonwamboldt.ca/understanding-the-php-fpm-status-page-1603/

formatting changes

2d18f9d

Now adhering to goimports

fix unicode chars in fields.yml

1f99808

UnicodeEncodeError: 'ascii' codec can't encode character u'\u2026' in position 41: ordinal not in range(128)

make update metricbeat

d4015cb

Using make on WSL (Windows 10 + Ubuntu)

Update CHANGELOG

de89192

Update assets file after rebase from elastic:master

d078f5c

Added new metricset "process" to "php_fpm"

44683bd

This emits separate events for each process in the pool "processes" list.

Update /include/fields.go

c9af02c

thanks @jsoriano for elastic#8466

narph force-pushed the php-fpm-full branch from a04908e to c9af02c Compare September 27, 2018 14:49

Updated config.reference to disable non-default metricset

be6e013

Update field types to "keyword" and "text" inside fields.yml

a27d648

Merge branch 'master' into php-fpm-full

2de150c

jsoriano merged commit 184793f into elastic:master Sep 29, 2018

jsoriano added needs_backport PR is waiting to be backported to other branches. v6.5.0 labels Sep 29, 2018

jsoriano mentioned this pull request Oct 18, 2018

Cherry-pick #8394 to 6.x: Add support for full php-fpm status (#7529) #8645

Merged

jsoriano removed the needs_backport PR is waiting to be backported to other branches. label Oct 18, 2018

jsoriano added a commit that referenced this pull request Oct 18, 2018

Add support for full php-fpm status (#7529) (#8394) (#8645)

c1a3472

Add new `process` metricset to collect list of processes in php fpm pools using the full status report. (cherry picked from commit 184793f)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for full php-fpm status (#7529) #8394

Add support for full php-fpm status (#7529) #8394

narph commented Sep 21, 2018 •

edited

Loading

ruflin Sep 24, 2018

narph Sep 24, 2018 •

edited

Loading

ruflin commented Sep 24, 2018

jsoriano left a comment

jsoriano Sep 25, 2018 •

edited

Loading

jsoriano left a comment

jsoriano Sep 26, 2018

narph Sep 26, 2018

jsoriano Sep 26, 2018

narph Sep 26, 2018

jsoriano Sep 26, 2018

jsoriano Sep 26, 2018

jsoriano left a comment

jsoriano Sep 27, 2018

narph Sep 28, 2018

jsoriano commented Sep 28, 2018

Add support for full php-fpm status (#7529) #8394

Add support for full php-fpm status (#7529) #8394

Conversation

narph commented Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

narph Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

ruflin commented Sep 24, 2018

jsoriano left a comment

Choose a reason for hiding this comment

jsoriano Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

jsoriano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoriano commented Sep 28, 2018

narph commented Sep 21, 2018 •

edited

Loading

narph Sep 24, 2018 •

edited

Loading

jsoriano Sep 25, 2018 •

edited

Loading