New PHP-FPM metricbeat module #3415

tsouza · 2017-01-19T16:37:43Z

This adds PHP-FPM support for metricbeat (#2247)

It is my first attempt writing a metricbeat module. One minor configuration that is not clear to me is how to externalize configuration of DefaultPath of the HostParser. Currently it is set to /status (which is php-fpm's default) but I just don't know how to make it user-configurable.

Also, I believe metricbeat creates sample dashboards, I would like to build a php-fpm sample dashboard and add it. Where to start?

ruflin · 2017-01-19T18:58:52Z

@tsouza I recommend you to run make fmt and make update before the next commit. This will clean up the source code and update all generated files. This should get you closer to a green build :-)

Add pool start time and start since stats Add support for proc stats Refactor stats api call Fix listen_queue => listen_queue_len" Revert "Fix listen_queue => listen_queue_len"" This reverts commit 46ff489fa8cce8d73c2aba42758a51bde4d68a77. Add suport for "listen_queue_len" Add documentation Minor const refactor Add generated doc Add basic testing Fix illegal char in asciidoc Fix type Update generated files Fix package name Add integration tests

ruflin · 2017-01-20T08:16:03Z

@tsouza Travis failures are unrelated to your PR. They should be fixed as soon as #3425 is merged.

ruflin

I left you a few notes mainly on the schema of the events.

It would be nice to have a simple system tests that checks if docs and the generate events are in sync. See https://github.com/elastic/beats/blob/master/metricbeat/tests/system/test_mysql.py#L41 as an example.

Let me know if I can help in any way.

ruflin · 2017-01-20T08:17:26Z

metricbeat/docs/modules/php_fpm.asciidoc

+  metricsets: ["pool"]
+  enabled: true
+  period: 10s
+  hosts: ["localhost"]


What is the default port the metrics are exposed? 80 or is it 9000 ?

php-fpm default port is 9000 and default status endpoint is /status. But these defaults does matter much since it's fastcgi protocol and can't be accessed directly. So there is a need to configure a proxy (with fastcgi support) and that can be any port and path (no defaults)

Hm, problem is with the above port 80 is the default. Not sure what we should set here best.

Maybe we can use the "default non-default" 8080 (which is at least nginx's default, iirc)

ruflin · 2017-01-20T08:19:01Z

metricbeat/module/php_fpm/_meta/config.yml

@@ -0,0 +1,6 @@
+- module: php_fpm


We currently have all module configs commented out which are not enabled by default. This might change in the near future, but for consistency it should be commented out here

The module I started looking at to learn is haproxy, which is no commented out. But I'll do it then!

Ah, haproxy has a config.yml and a config.full.yml, that is why it also works there.

ruflin · 2017-01-20T08:20:41Z

metricbeat/module/php_fpm/_meta/fields.yml

+  title: "php_fpm"
+  description: >
+    PHP-FPM server status metrics collected from PHP-FPM.
+  fields:


Can you add here the experimental[] flag? We do that for all new modules so we can still break compatibility after the first release. This might be a beta flag soonish.

Also please add short_config: false so it does not show up in the short config. See https://github.com/elastic/beats/blob/master/metricbeat/module/prometheus/_meta/fields.yml#L7 as example.

I think I forgot to mention it in the review, but also the metricsets should have a log line in the New(...) part that has a warning that it is experimental. Also see prometheus.

ruflin · 2017-01-20T08:22:43Z

metricbeat/module/php_fpm/php_fpm.go

+// HostParser is used for parsing the configured php-fpm hosts.
+var HostParser = parse.URLHostParserBuilder{
+	DefaultScheme: defaultScheme,
+	DefaultPath:   defaultPath,


To define a config option to change the defaultPath you can use PathConfigKey. See https://github.com/elastic/beats/blob/master/metricbeat/module/prometheus/collector/collector.go#L25 as an example. I would suggest to use path or status_path as the config option. All other parts are done automatically by the HostParser.

ruflin · 2017-01-20T08:25:40Z

metricbeat/module/php_fpm/pool/pool.go

+// configuration entries if needed.
+func New(base mb.BaseMetricSet) (mb.MetricSet, error) {
+
+	config := struct{}{}


You can remove the config part as it is empty in your case.

ruflin · 2017-01-20T08:34:59Z

metricbeat/module/php_fpm/pool/_meta/fields.yml

+      description: >
+        The size of the socket queue of pending connections.
+
+    - name: idle_processes


processes.idle

ruflin · 2017-01-20T08:35:06Z

metricbeat/module/php_fpm/pool/_meta/fields.yml

+      description: >
+        The number of idle processes.
+
+    - name: active_processes


processes.active

ruflin · 2017-01-20T08:35:11Z

metricbeat/module/php_fpm/pool/_meta/fields.yml

+      description: >
+        The number of active processes.
+
+    - name: total_processes


processes.total

ruflin · 2017-01-20T08:35:20Z

metricbeat/module/php_fpm/pool/_meta/fields.yml

+      description: >
+        The number of idle + active processes.
+
+    - name: max_active_processes


processes.max_active

ruflin · 2017-01-20T08:36:14Z

metricbeat/module/php_fpm/php_fpm.go

+}
+
+// PoolStats defines all stats fields from a php-fpm pool
+type PoolStats struct {


If the response is in JSON, you could use the schema package to do the conversion. But we can also do that in a follow up PR.

Indeed the response is in JSON and also it came to me that there would be a better way of handling this situation. But I wonder what will happen to the field names itself, since php-fpm replies a JSON with whitespace in field names (e.g. accepted conn)

Mind pointing out an implementation that uses it?

Not directly JSON, but similar: https://github.com/elastic/beats/blob/master/metricbeat/module/mongodb/status/data.go

andrewkroh

Nice job!

andrewkroh · 2017-01-20T14:24:22Z

metricbeat/module/php_fpm/php_fpm.go

+// NewStatsClient creates a new StatsClient
+func NewStatsClient(m mb.BaseMetricSet, isFullStats bool) *StatsClient {
+	var address string
+	address = m.HostData().SanitizedURI + "?json"


You can move the query param to the URLHostParserBuilder by setting QueryParams: "json".

You can just write address := m.HostData().SanitizedURI and drop the var address string. (https://golang.org/ref/spec#Short_variable_declarations)

andrewkroh · 2017-01-20T14:31:22Z

metricbeat/module/php_fpm/php_fpm.go

+
+// Fetch php-fpm stats
+func (c *StatsClient) Fetch() (io.ReadCloser, error) {
+	req, err := http.NewRequest("GET", c.address, nil)


Once #3413 merges we should update this to use it.

Actually, I was thinking of skipping the http proxy and directly access php-fpm using a fastcgi client. I could find this one go-fastcgi-client (since golang's fcgi package is for server only). What do you think?

tsouza · 2017-01-21T20:03:42Z

The current metric names and it's description was copied directly from php-fpm configuration file. As they do not follow beats convention much (as pointed out by @ruflin), I decided to take a closer look to find proper names.

It seems that the process metrics regarding request IMO are useless. The metrics request duration, request method and request uri are from the current request at the time the status endpoint was invoked (which might temporarily become the last, if no request is being served by the process). Whereas the last request cpu/memory are calculated only after the request is done. Thus, in a running system serving several requests per second, there won't be any real correlation between last request cpu/memory and those request* metrics (or might be just too difficult to correlate).

So I am worried about that users might start to correlate between request uri and last request cpu, which might be misleading. This makes me wonder how useful those last request cpu/memory metrics might actually be (as the user could actually correlate with data generated by process metricbeat).

ruflin · 2017-01-23T10:19:26Z

@tsouza If possible I would suggest to leave out calculated values as this can also be potentially done on the elasticsearch side. The way you describe it above it seems even to be temporary so not too usefule as you said.

To move this PR forward I suggest to remove the values you are not sure about are useful. The hard part of doing a Metricset is getting all the data and do the conversion. Adding a field is fairly easy. So in case someone requests it, we get the opportunity to ask why it is needed and how it is used and we can easily add it. WDYT?

tsouza · 2017-01-24T03:11:30Z

@ruflin Agreed. I'll just keep the minimum set of metrics that I judge useful.

Also, thanks to this article for providing a much better documentation. I am leaving here for future reference.

ruflin · 2017-01-24T06:16:03Z

metricbeat/module/php_fpm/pool/pool.go

+		"hostname": m.Host(),
+
+		"pool":                 stats.Pool,
+		"connections.queued":   stats.ListenQueue,


You have to nest this into a common.MapStr as otherwise elasticsearch 2.x will complain here. So this becomes:

"connections": common.MapStr{ "queued": stats.ListenQueue, "accepted" : stats.AcceptdConn, }

Ok! The docs was not clear in that sense to me! So I just took a shot hah

I think we actually detected this problem with 2.x only after we wrote the docs ;-)

* Add docker environment for integration and system testing * Add system test file to check for correct docs. * Brings docs in line with generated output * Update data.json * Remove hostname fields as already part of metricset * Apply schema instead of manual conversion * Rename pool.pool to pool.name * Remove separate http client as not needed anymore This is a follow up PR for elastic#3415

* Add docker environment for integration and system testing * Add system test file to check for correct docs. * Brings docs in line with generated output * Update data.json * Remove hostname fields as already part of metricset * Apply schema instead of manual conversion * Rename pool.pool to pool.name * Remove separate http client as not needed anymore This is a follow up PR for #3415

* Add docker environment for integration and system testing * Add system test file to check for correct docs. * Brings docs in line with generated output * Update data.json * Remove hostname fields as already part of metricset * Apply schema instead of manual conversion * Rename pool.pool to pool.name * Remove separate http client as not needed anymore This is a follow up PR for elastic#3415

tsouza added Metricbeat Metricbeat module labels Jan 19, 2017

ruflin added the in progress Pull request is currently in progress. label Jan 19, 2017

tsouza force-pushed the metricset-php_fpm branch from 212dcfc to 539f229 Compare January 19, 2017 21:45

ruflin reviewed Jan 20, 2017

View reviewed changes

andrewkroh approved these changes Jan 20, 2017

View reviewed changes

Thiago Souza added 3 commits January 21, 2017 14:08

PR review: refactor data structures

7047806

PR review: minor changes

4f87ead

PR review: use HostParser.QueryString

3de7963

Thiago Souza added 5 commits January 24, 2017 01:25

PR review: no need for "var address"

a36196f

PR review: remove proc metricset (metrics does not seems really useful)

60bba3a

PR review: reduced metric set to a bare minimum

7294c40

Generate artifacts

9bc8a1a

Remove uneeded bool flag

28352de

ruflin reviewed Jan 24, 2017

View reviewed changes

PR review: add structures

f7ea5e6

ruflin added review and removed in progress Pull request is currently in progress. labels Jan 24, 2017

ruflin merged commit 58ed51f into elastic:master Jan 24, 2017

ruflin mentioned this pull request Jan 24, 2017

Improve PHP-PFM module #3450

Merged

tsouza deleted the metricset-php_fpm branch January 25, 2017 18:50

monicasarbu mentioned this pull request Jan 30, 2017

Adding php-fpm module to metricbeat #2247

Closed

New PHP-FPM metricbeat module #3415

New PHP-FPM metricbeat module #3415

Conversation

tsouza commented Jan 19, 2017 • edited Loading

ruflin commented Jan 19, 2017

ruflin commented Jan 20, 2017

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsouza Jan 20, 2017 • edited Loading

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

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

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

andrewkroh 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

tsouza commented Jan 21, 2017 • edited Loading

ruflin commented Jan 23, 2017

tsouza commented Jan 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsouza commented Jan 19, 2017 •

edited

Loading

tsouza Jan 20, 2017 •

edited

Loading

tsouza commented Jan 21, 2017 •

edited

Loading

tsouza commented Jan 24, 2017 •

edited

Loading