new plugin: nginx_plus #3214

poblahblahblah · 2017-09-09T02:20:22Z

Required for all PRs:

Signed CLA.
Associated README.md updated.
Has appropriate unit tests.

poblahblahblah · 2017-09-09T02:21:57Z

@danielnelson Here's a copy that compiles but needs a bit of cleanup. Before I dig deeper it would be nice if you could give it a quick once over. I'll clean up my WIP commits once we're ready to actually merge.

@mplonka here's a new PR retaining your original commits

poblahblahblah · 2017-09-09T02:22:59Z

plugins/inputs/nginx_plus/README.md

+
+### Measurements & Fields:
+
+- Measurement


This needs to be cleaned up to reflect what nginx plus will give us.

poblahblahblah · 2017-09-09T02:23:11Z

plugins/inputs/nginx_plus/README.md

+### Tags:
+
+- All measurements have the following tags:
+    - port


This probably needs to get cleaned up.

poblahblahblah · 2017-09-09T02:23:53Z

plugins/inputs/nginx_plus/nginx_plus.go

+	}
+	contentType := strings.Split(resp.Header.Get("Content-Type"), ";")[0]
+	switch contentType {
+	case "text/plain":


This is a holdover from the previous PR - this should probably just be removed as the nginx plus status module will just return JSON.

Yeah, just try to parse and don't worry about this being set correctly.

poblahblahblah · 2017-09-09T02:32:14Z

plugins/inputs/nginx_plus/nginx_plus.go

+	"time"
+
+	"github.com/influxdata/telegraf"
+	"github.com/influxdata/telegraf/internal/errchan"


I was working off an old master, so I will need to rebase and get rid of all calls to errchan.

poblahblahblah · 2017-09-09T16:00:35Z

plugins/inputs/nginx_plus/README.md

+
+### Measurements & Fields:
+
+- nginx_processes


@danielnelson should these be renamed to nginx_plus_<measurement> ?

Yeah, it should be easy for anyone to change the prefix with name_override if they prefer the shorter name.

Also removing code that was specific to regular nginx. The nginx plus status url should always return JSON.

danielnelson

Looking good

danielnelson · 2017-09-12T23:57:12Z

plugins/inputs/nginx_plus/README.md

+### Reference material
+
+Structures for Nginx Plus have been built based on history of
+[status module documentation](http://nginx.org/en/docs/http/ngx_http_status_module.html)


Lets move this up to the first section, and mention that you need a commercial license to have access to this module.

danielnelson · 2017-09-13T00:01:02Z

plugins/inputs/nginx_plus/nginx_plus.go

+var client = &http.Client{
+	Transport: tr,
+	Timeout:   time.Duration(4 * time.Second),
+}


Make the Client and Transport part of the NginxPlus struct, this way they won't need to share configuration values if you have more than one plugin instance.

Bonus points if you want to add SSL support and a timeout option, but not required.

poblahblahblah · 2017-09-13T01:09:47Z

@danielnelson Updated PR per feedback and added a timeout. My golang-fu isn't too great so I would rather tackle SSL support in a follow up PR, if possible.

danielnelson · 2017-09-14T00:41:06Z

plugins/inputs/nginx_plus/nginx_plus.go

+
+	client := &http.Client{
+		Timeout: n.ResponseTimeout.Duration,
+	}


Initialize the client only once, probably above since it can be shared between goroutines. Use a new http.Transport, I'd just follow the original nginx plugin here: https://github.com/influxdata/telegraf/blob/master/plugins/inputs/nginx/nginx.go#L63-L69. If you don't have TLS support then the transport can be initialized like &Transport{}

The reason for this is that each http.Transport/http.Client comes with it's own connection pool and this will allow the connection to be kept alive between Gather intervals.

danielnelson · 2017-09-14T00:47:22Z

plugins/inputs/nginx_plus/README.md

+  - upstream
+  - server
+  - port
+  - serverAddress


This should be server_address to match the naming style. This is the address of the upstream? If so maybe it would be more descriptive to call it upstream_address?

danielnelson · 2017-09-14T00:49:36Z

plugins/inputs/nginx_plus/README.md

+
+- nginx_plus_upstream_peer, nginx_plus_stream_upstream_peer
+  - id
+  - host


Don't list host since it is added globally by Telegraf (depending on config).

danielnelson · 2017-09-14T00:53:17Z

plugins/inputs/nginx_plus/README.md

+It produces:
+```
+* Plugin: inputs.nginx_plus, Collection 1
+> nginx_plus_processes,server=localhost,port=12021,host=word.local respawned="0xc420075538" 1504922954000000000


respawned is an address, this is probably a bug where we need to dereference a pointer.

@danielnelson I think I fixed all of these. Here's the commit: 33fe1f0 - It looks like in the original commits the test was written like this, which was making the tests pass:

"selected": func() *int64 { var v int64 = 1451606400000 return &v }(),

I updated the test to just expect the int64:

"selected": int64(1451606400000),

And updated the map:

"selected": *peer.Selected,

Those were the only two instances where this was happening. I made the changes to both nginx_plus.go and the tests, and made sure this compiled. I then ran telegraf in test mode against one of our load balancers and I did not see any addresses in the output.

If this looks like a good fix I will update the README to remove the addresses from the sample output.

You will want to check that they are not nil before dereferencing, I assume they are pointers because in some versions they don't exist.

How would you recommend I do that? Something like this for respawned?

func (s *Status) gatherProcessesMetrics(tags map[string]string, acc telegraf.Accumulator) { var respawned int if s.Processes.Respawned != nil { respawned = *s.Processes.Respawned } acc.AddFields( "nginx_plus_processes", map[string]interface{}{ "respawned": respawned, }, tags, ) }

Pushed a fix in c84861a

danielnelson · 2017-09-14T00:53:51Z

plugins/inputs/nginx_plus/README.md

+> nginx_plus_ssl,server=localhost,port=12021,host=word.local handshakes=0i,handshakes_failed=0i,session_reuses=0i 1504922954000000000
+> nginx_plus_requests,host=word.local,server=localhost,port=12021 total=147885504244i,current=10019i 1504922954000000000
+> nginx_plus_upstream,host=word.local,upstream=dataserver80,server=localhost,port=12021 zombies=0i,keepalive=0i 1504922954000000000
+> nginx_plus_upstream_peer,id=0,server=localhost,port=12021,host=word.local,upstream=dataserver80,serverAddress=10.10.102.181:80 responses_5xx=27831i,healthchecks_unhealthy=1i,downtime=484817i,healthchecks_last_passed=true,responses_1xx=0i,active=22i,requests=2620930i,responses_total=2620652i,fails=4i,downstart=0i,state="up",responses_4xx=16i,healthchecks_checks=14133i,selected="0xc4201b22e8",response_time=95i,responses_2xx=2592805i,weight=1i,responses_3xx=0i,sent=3802831967i,received=536695496i,unavail=4i,healthchecks_fails=27i,header_time=94i,backup=false 1504922954000000000


selected also has an address and probably needs dereferenced. Double check that other similar pointers are dereferenced.

danielnelson · 2017-09-19T18:51:39Z

This will be in 1.5, thanks @poblahblahblah @mplonka

poblahblahblah commented Sep 9, 2017

View reviewed changes

mplonka added 4 commits September 9, 2017 08:24

Nginx Plus stats

0f772a3

applying code review suggestions

02eb8cd

fixing test data

74374cb

stream session stats reuse struct for http response stats

079ff75

poblahblahblah force-pushed the nginx-plus branch from 3372ebd to d204a2f Compare September 9, 2017 15:38

poblahblahblah added 2 commits September 9, 2017 08:40

untangle nginx_plus from nginx

579952e

do not use errchan

5987e64

poblahblahblah force-pushed the nginx-plus branch from d204a2f to 5987e64 Compare September 9, 2017 15:40

Updating README with correct measurements, fields, and tags

c95dad2

poblahblahblah commented Sep 9, 2017

View reviewed changes

poblahblahblah added 2 commits September 9, 2017 09:06

Missed a few measurements

6bfc295

renaming measurements from nginx_ to nginx_plus_

f3d0931

Also removing code that was specific to regular nginx. The nginx plus status url should always return JSON.

danielnelson added this to the 1.5.0 milestone Sep 12, 2017

danielnelson reviewed Sep 13, 2017

View reviewed changes

Moving Client to NginxPlus Struct; Adding timeout

f965bca

poblahblahblah force-pushed the nginx-plus branch from cdff0ea to f965bca Compare September 13, 2017 01:11

danielnelson reviewed Sep 14, 2017

View reviewed changes

poblahblahblah added 3 commits September 14, 2017 07:53

Initialize client only once

767fd53

fix two pointer dereference issues

33fe1f0

Check pointers for nil before dereferencing

c84861a

danielnelson added the new plugin label Sep 18, 2017

Updating output example in README.md

b19a1da

poblahblahblah force-pushed the nginx-plus branch from 3e2179d to b19a1da Compare September 19, 2017 00:59

danielnelson merged commit 6e6ed07 into influxdata:master Sep 19, 2017

danielnelson mentioned this pull request Sep 19, 2017

Nginx Plus stats #2405

Closed

maxunt pushed a commit that referenced this pull request Jun 26, 2018

Add new nginx_plus input plugin (#3214)

d3cb050

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new plugin: nginx_plus #3214

new plugin: nginx_plus #3214

poblahblahblah commented Sep 9, 2017 •

edited by danielnelson

Loading

poblahblahblah commented Sep 9, 2017

poblahblahblah Sep 9, 2017

poblahblahblah Sep 9, 2017

poblahblahblah Sep 9, 2017

danielnelson Sep 12, 2017

poblahblahblah Sep 9, 2017

danielnelson Sep 12, 2017

poblahblahblah Sep 9, 2017

danielnelson Sep 12, 2017

danielnelson left a comment

danielnelson Sep 12, 2017

danielnelson Sep 13, 2017

poblahblahblah commented Sep 13, 2017

danielnelson Sep 14, 2017

danielnelson Sep 14, 2017

danielnelson Sep 14, 2017

danielnelson Sep 14, 2017

poblahblahblah Sep 14, 2017 •

edited

Loading

danielnelson Sep 14, 2017

poblahblahblah Sep 14, 2017 •

edited

Loading

danielnelson Sep 15, 2017

poblahblahblah Sep 15, 2017

danielnelson Sep 14, 2017

danielnelson commented Sep 19, 2017

new plugin: nginx_plus #3214

new plugin: nginx_plus #3214

Conversation

poblahblahblah commented Sep 9, 2017 • edited by danielnelson Loading

Required for all PRs:

poblahblahblah commented Sep 9, 2017

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

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

poblahblahblah commented Sep 13, 2017

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

poblahblahblah Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poblahblahblah Sep 14, 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

danielnelson commented Sep 19, 2017

poblahblahblah commented Sep 9, 2017 •

edited by danielnelson

Loading

poblahblahblah Sep 14, 2017 •

edited

Loading

poblahblahblah Sep 14, 2017 •

edited

Loading