Nginx Plus stats #2405

mplonka · 2017-02-14T09:56:45Z

Retrieves Nginx Plus stats from "stats" module.

phemmer · 2017-02-14T13:53:29Z

Just my own $0.02: The vast majority of the code seems to be mapping of data structures. What fields to look for in the nginx output, and then what fields to pass on, and how to name them.
I would encourage a more programmatic approach. I've been bitten by several plugins in telegraf that use hard-coded field mappings because either a new version of the software was released and telegraf hadn't been updated to support the fields, or the plugin author didn't think the fields were useful.

Also what about merging some of the measurements together, such as nginx.requests, nginx.ssl, nginx.connections and nginx.processes. This way were not overwhelming the user with a bunch of micro-measurements. For some of them they should be kept separate, because there are multiple series (tag permutations), but for the 4 mentioned, there's only one series in all of them.

But again, this is just my personal thoughts. I'm not a maintainer, so they should not be construed as requirements. :-)

mplonka · 2017-02-14T14:02:44Z

Thanks for your comments, PHemmer. I'm happy to modify the code to make it more consistent with other plugins and simplify this data mapping there. Is there any reference plugin that I should use as a reference?

poblahblahblah · 2017-07-19T00:37:19Z

Hello,

We would like to see nginx+ monitoring in telegraf. What needs to be done to get this revived and merged in? Is there a good reference plugin I can look at to address the comments @phemmer had? Do the telegraf maintainers preferences mirror those comments?

danielnelson

I'm a little more open to this style of parsing, though I agree it would be nice for it to be less code. I think a good first step would be to extract and reuse some of anonymous structs. This might open things up for using more helper functions. I'll comment on some lines where I see this in the code.

The other thing I would like to see is for this to become a standalone plugin since it doesn't really share much code with the nginx plugin, scraps a different plugin, and provides completely different points.

danielnelson · 2017-07-19T00:56:46Z

plugins/inputs/nginx/nginxplus.go

+			Bytes            int64 `json:"bytes"`
+			ResponsesWritten int64 `json:"responses_written"`
+			BytesWritten     int64 `json:"bytes_written"`
+		} `json:"bypass"`


I'd like to see all of the anonymous structs here that have responses and bytes using a shared stuct.

danielnelson · 2017-07-19T00:57:36Z

plugins/inputs/nginx/nginxplus.go

+				Responses4xx int64 `json:"4xx"`
+				Responses5xx int64 `json:"5xx"`
+				Total        int64 `json:"total"`
+			} `json:"responses"`


The Responses struct here could probably be extracted to a named struct.

danielnelson · 2017-07-19T00:58:06Z

plugins/inputs/nginx/nginxplus.go

+					Fails      int64 `json:"fails"`
+					Unhealthy  int64 `json:"unhealthy"`
+					LastPassed *bool `json:"last_passed"`
+				} `json:"health_checks"`


The HealthCheck struct also appears in another location.

danielnelson · 2017-07-19T01:00:20Z

plugins/inputs/nginx/nginxplus.go

+
+func (s *Status) gatherProcessesMetrics(tags map[string]string, acc telegraf.Accumulator) {
+	acc.AddFields(
+		"nginx.processes",


Anywhere we are using dotted notation should either use snake_case or in some cases the subitems should become tags. I think in this PR all should become snake_case.

This applies to measurement names and field names throughout the pull request.

danielnelson · 2017-07-19T01:01:46Z

plugins/inputs/nginx/nginxplus.go

+5. http://web.archive.org/web/20150414043916/http://nginx.org/en/docs/http/ngx_http_status_module.html
+6. http://web.archive.org/web/20150918163811/http://nginx.org/en/docs/http/ngx_http_status_module.html
+7. http://web.archive.org/web/20161107221028/http://nginx.org/en/docs/http/ngx_http_status_module.html
+*/


This comment would go into the new nginxplus README

mplonka · 2017-07-20T15:49:26Z

Hi @danielnelson. Thanks for the review. Please let me know if this one looks better.

danielnelson

Looks good, but would still like to see it as a new plugin.

danielnelson · 2017-07-21T18:21:15Z

plugins/inputs/nginx/nginxplus.go

+				Sessions3xx int64 `json:"3xx"`
+				Sessions4xx int64 `json:"4xx"`
+				Sessions5xx int64 `json:"5xx"`
+			} `json:"sessions"`


I think this can be a HttpResponsesStats

100%
Renaming this struct to make the name still meaningful in this context

danielnelson · 2017-07-21T18:28:37Z

plugins/inputs/nginx/nginxplus.go

+		Current int   `json:"current"`
+	} `json:"requests"`
+
+	ServerZones map[string]struct { // added in version 2


Does this need to be a pointer since it is not in version 1?

It's completely absent from version 1

danielnelson · 2017-07-21T18:28:44Z

plugins/inputs/nginx/nginxplus.go

+		} `json:"queue"`
+	} `json:"upstreams"`
+
+	Caches map[string]struct { // added in version 2


Does this need to be a pointer since it is not in version 1?

Same as above: absent from version 1

danielnelson · 2017-07-21T18:29:13Z

plugins/inputs/nginx/README.md

+
+- [version 6](http://web.archive.org/web/20150918163811/http://nginx.org/en/docs/http/ngx_http_status_module.html)
+
+- [version 7](http://web.archive.org/web/20161107221028/http://nginx.org/en/docs/http/ngx_http_status_module.html)


How does someone know what version they have? Does this plugin support all versions?

We can probably cover all the version by linking to https://nginx.org/en/docs/http/ngx_http_status_module.html#compatibility

The plugin supports all versions. Version is sent in the very first attribute.

mplonka · 2017-07-21T22:02:32Z

I do not have strong views on adding Nginx Plus support to this plugin or creating separate one. Keeping it here seems more intuitive for Nginx Plus users though.

danielnelson · 2017-07-21T22:12:11Z

Please make it a new plugin nginxplus. The reason for this is that it has unique setup and format from the regular nginx so there is little to gain from combining them.

poblahblahblah · 2017-08-14T23:57:30Z

Hey @mplonka - can I help in any way get this plugin separated out into a separate plugin?

poblahblahblah · 2017-09-09T00:09:51Z

Hello,

Is there anything I can do to get this separated out into a separate plugin?

danielnelson · 2017-09-09T00:25:53Z

@poblahblahblah Could you fork @mplonka's code and update the plugin per the review?

poblahblahblah · 2017-09-09T01:22:27Z

@danielnelson Would you like these two plugins to be completely separate? It looks like the current way it's written the nginxplus plugin use a number of functions from the nginx plugin. I can separate these completely or I can just import the existing nginx plugin into nginxplus.

Any guidance would be appreciated :)

danielnelson · 2017-09-09T01:44:48Z

I think they should probably be completely independent. I didn't see much shared code, is there something specific you are looking at?

danielnelson · 2017-09-19T18:48:21Z

Added in #3214

Nginx Plus stats

7cb28fb

sparrc added this to the 1.3.0 milestone Feb 14, 2017

danielnelson modified the milestones: 1.4.0, 1.3.0 Apr 13, 2017

danielnelson reviewed Jul 19, 2017

View reviewed changes

applying code review suggestions

d03419c

danielnelson reviewed Jul 21, 2017

View reviewed changes

mplonka added 2 commits July 21, 2017 21:11

fixing test data

6a6a123

stream session stats reuse struct for http response stats

0abf403

danielnelson modified the milestones: 1.4.0, 1.5.0 Aug 14, 2017

danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Aug 24, 2017

danielnelson added the new plugin label Sep 18, 2017

danielnelson closed this Sep 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nginx Plus stats #2405

Nginx Plus stats #2405

mplonka commented Feb 14, 2017

phemmer commented Feb 14, 2017

mplonka commented Feb 14, 2017

poblahblahblah commented Jul 19, 2017

danielnelson left a comment

danielnelson Jul 19, 2017

danielnelson Jul 19, 2017

danielnelson Jul 19, 2017

danielnelson Jul 19, 2017

danielnelson Jul 19, 2017

mplonka commented Jul 20, 2017

danielnelson left a comment

danielnelson Jul 21, 2017

mplonka Jul 21, 2017

danielnelson Jul 21, 2017

mplonka Jul 21, 2017

danielnelson Jul 21, 2017

mplonka Jul 21, 2017

danielnelson Jul 21, 2017

danielnelson Jul 21, 2017

mplonka Jul 21, 2017

mplonka commented Jul 21, 2017

danielnelson commented Jul 21, 2017

poblahblahblah commented Aug 14, 2017

poblahblahblah commented Sep 9, 2017

danielnelson commented Sep 9, 2017

poblahblahblah commented Sep 9, 2017

danielnelson commented Sep 9, 2017

danielnelson commented Sep 19, 2017


		- [version 6](http://web.archive.org/web/20150918163811/http://nginx.org/en/docs/http/ngx_http_status_module.html)

		- [version 7](http://web.archive.org/web/20161107221028/http://nginx.org/en/docs/http/ngx_http_status_module.html)

Nginx Plus stats #2405

Nginx Plus stats #2405

Conversation

mplonka commented Feb 14, 2017

phemmer commented Feb 14, 2017

mplonka commented Feb 14, 2017

poblahblahblah commented Jul 19, 2017

danielnelson 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

mplonka commented Jul 20, 2017

danielnelson 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mplonka commented Jul 21, 2017

danielnelson commented Jul 21, 2017

poblahblahblah commented Aug 14, 2017

poblahblahblah commented Sep 9, 2017

danielnelson commented Sep 9, 2017

poblahblahblah commented Sep 9, 2017

danielnelson commented Sep 9, 2017

danielnelson commented Sep 19, 2017