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

WIP: Scrape json metrics from nginx vts #325

Closed
wants to merge 1 commit into from

Conversation

gianrubio
Copy link
Contributor

@gianrubio gianrubio commented Feb 22, 2017

This PR add the ability to ingress scrape nginx-vts and convert json format to prometheus. I'm still working on this, it's one of my firsts golang codes so reviews are welcome.

Screenshot

screen shot 2017-02-22 at 22 55 26

TODO

  • review ingress docs
  • write tests

Fixes #72, #305, #239

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 22, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@aledbf
Copy link
Member

aledbf commented Feb 22, 2017

@gianrubio we need two implementations, one for the default nginx status info and another for vts. This must be optional. We cannot force users to use the vts module and the default information does not contains all the information vts provides.

@gianrubio gianrubio changed the title WIP: Scrap json metrics from nginx vts WIP: Scrape json metrics from nginx vts Feb 23, 2017
@gianrubio
Copy link
Contributor Author

@aledbf got it! I'll do that

@rikatz
Copy link
Contributor

rikatz commented Feb 24, 2017

Don't know if this is related, but we're using this as a sidecar container here: https://github.com/hnlq715/nginx-vts-exporter

So my question is, wouldn't be simplier to create a example deployment POD, with ingress + the vts exporter?

@gianrubio
Copy link
Contributor Author

@rikatz we already discussed this, take a look on #72

@@ -40,7 +42,10 @@ func (em exeMatcher) MatchAndName(nacl common.NameAndCmdline) (bool, string) {
}

func (n *NGINXController) setupMonitor(args []string) {
pc, err := newProcessCollector(true, exeMatcher{"nginx", args})
var enableVts = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aledbf when the nginxController starts the configmap are empty. Could I change the enable-vts-metrics from annotation to an argument?

Copy link
Member

Choose a reason for hiding this comment

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

@gianrubio please no. Keep in mind we must support to switch the implementation if the enable-vts-metrics value is updated in the configmap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so what method is the recommend to call the setupMonitor? Is the reload method?

func (n NGINXController) Reload(data []byte) ([]byte, bool, error) {

Copy link
Member

Choose a reason for hiding this comment

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

@gianrubio I think after this line is the most appropriate place.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.3%) to 44.452% when pulling 8eced17aca208af5fab12e7dab3813a9c498f60d on gianrubio:ngx-vts-prometheus into 0aabfba on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.4%) to 44.416% when pulling 645509e80102a5c295029e3d66cea033cca6f23a on gianrubio:ngx-vts-prometheus into 0aabfba on kubernetes:master.

@gianrubio gianrubio force-pushed the ngx-vts-prometheus branch 7 times, most recently from 10b7de2 to 3fc59b9 Compare March 9, 2017 22:05
upgrade vts to the latest version
@gianrubio gianrubio force-pushed the ngx-vts-prometheus branch from 3fc59b9 to 6db5c9e Compare March 9, 2017 22:06
@gianrubio
Copy link
Contributor Author

@aledbf could you review again?

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.2%) to 45.082% when pulling 6db5c9e on gianrubio:ngx-vts-prometheus into db96c9d on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.3%) to 45.047% when pulling 6db5c9e on gianrubio:ngx-vts-prometheus into db96c9d on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.2%) to 45.082% when pulling 6db5c9e on gianrubio:ngx-vts-prometheus into db96c9d on kubernetes:master.

@aledbf aledbf closed this Mar 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants