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

Add Nginx Plus status #2329

Closed
wants to merge 2 commits into from
Closed

Conversation

mrkschan
Copy link

No description provided.

@mrkschan
Copy link
Author

This is a WIP PR. Please let me know if the big chunk of copy-and-paste for reading the JSON from Nginx plus status endpoint is appropriate.

@elasticsearch-release
Copy link

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

1 similar comment
@elasticsearch-release
Copy link

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

}

// getURL constructs a URL from the rawHost value and path if one was not set in the rawHost value.
func getURL(statusPath, rawHost string) (*url.URL, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious if we could move this to the nginx module? Long term we should probably make this even a general helper to be used for all http metricsets.

@ruflin
Copy link
Member

ruflin commented Aug 22, 2016

Thanks for the additional metricsets. There is a new schema package which you could make use of to better parse the json documents: https://www.elastic.co/guide/en/beats/metricbeat/current/creating-metricsets.html#_parsing_and_normalizing_fields For an example have a look at the filebeat metricset. Let me know if you need some more details. In general the MetricSets should define all the fields that they are sending so we can make sure the types are correct.

It seems there is quite a bit of duplicated code in the Metricsets. I would hope part of it could be moved to the nginx module and then be reused.

@ruflin ruflin added in progress Pull request is currently in progress. and removed review labels Aug 22, 2016
@ruflin
Copy link
Member

ruflin commented Sep 12, 2016

@mrkschan I just saw you pushed some additional commits. Can you ping me when it is ready for a review?

@mrkschan
Copy link
Author

@ruflin, I'm trying to apply Schema to Nginx plus stream.upstreams. I'm not sure what to do with the peers array in each "upstream". Find below a sample stats retrieved from https://demo.nginx.com/status/stream/upstreams.

"upstreams": {
     "dns_udp_backends" : {
            "peers" : [
                 {
                        "connections" : 226042,
                        "active" : 0,
                        "state" : "up",
                        "unavail" : 0,
                        "downstart" : 0,
                        "fails" : 0,
                        "downtime" : 0,
                        "weight" : 2,
                        "health_checks" : {
                             "last_passed" : true,
                             "checks" : 118399,
                             "fails" : 0,
                             "unhealthy" : 0
                        }
                 },
             :
             :

@ruflin
Copy link
Member

ruflin commented Sep 19, 2016

Arrays area always tricky. We have similar problems in other metricsets. So far we always tried to build a new metricset out of it that sends for each entry an event. That means in your case, each peer would be an event. If you go this way, you would probably have to add one more field to the peer which is type or something similar and contains for example unused_tcp_backends. The metricset could be called upstreampeer for example (I'm sure there are better name).

One of the questions that often helps to answer the question is on how this data will be consumed? What metrics is someone going to visualise? Will it be all peers with a specific ip and type? Or is it the number of peers per count? ...

@mrkschan mrkschan closed this Jan 4, 2017
@mrkschan
Copy link
Author

mrkschan commented Jan 4, 2017

Closing this ... will create a up-to-date PR later.

@ruflin
Copy link
Member

ruflin commented Jan 5, 2017

@mrkschan Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress Pull request is currently in progress. Metricbeat Metricbeat
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants