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

Annotations for the InfluxDB module #2505

Merged
merged 3 commits into from
May 21, 2018

Conversation

fntlnz
Copy link
Contributor

@fntlnz fntlnz commented May 13, 2018

Signed-off-by: Lorenzo Fontana lo@linux.com

What this PR does / why we need it:
This PR adds support for NGINX InfluxDB Module to the Nginx Ingress controller. The PR to update the image is #2533

This PR allows to configure the InfluxDB module using annotations.

This is useful for those who want to monitor every request passing through the ingress controller by sending them to InfluxDB or to one of the supported output of Telegraf (e.g Kafka, graylog, mqtt, opentsdb and all the others)

Special notes for your reviewer:

  • Please provide any hints! Need to know if I missed something
  • Still testing if all I made here works, please poiint out anything!

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 13, 2018
@aledbf
Copy link
Member

aledbf commented May 13, 2018

@fntlnz thank you for your contribution. Please split this PR in two. The first one updating build.sh and Makefile from the nginx directory and another adding the feature and updating the Makefile to use the new nginx image. Also please add e2e tests to verify the new feature

@fntlnz fntlnz changed the title Add the nginx influxdb module Annotations for the InfluxDB module May 17, 2018
@fntlnz
Copy link
Contributor Author

fntlnz commented May 17, 2018

Ok @aledbf I moved the image modifications to #2533

I also generated bindata, I don't know if I had to, just let me know. Not needed, after a rebase the bindata was not needed anymore.

@fntlnz fntlnz force-pushed the nginx-module branch 2 times, most recently from 1d8ac78 to c419206 Compare May 17, 2018 15:51
@codecov-io
Copy link

codecov-io commented May 17, 2018

Codecov Report

Merging #2505 into master will decrease coverage by 0.07%.
The diff coverage is 33.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2505      +/-   ##
==========================================
- Coverage    40.7%   40.62%   -0.08%     
==========================================
  Files          73       74       +1     
  Lines        5034     5093      +59     
==========================================
+ Hits         2049     2069      +20     
- Misses       2711     2743      +32     
- Partials      274      281       +7
Impacted Files Coverage Δ
internal/ingress/controller/config/config.go 98.23% <ø> (ø) ⬆️
internal/ingress/types_equals.go 12.41% <0%> (-0.09%) ⬇️
internal/ingress/controller/controller.go 2.23% <0%> (-0.02%) ⬇️
internal/ingress/annotations/annotations.go 80.95% <100%> (+0.3%) ⬆️
internal/ingress/controller/template/template.go 63.93% <28.57%> (-1.05%) ⬇️
internal/ingress/annotations/influxdb/main.go 38.46% <38.46%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa256ac...c3b896d. Read the comment docs.

@aledbf aledbf self-assigned this May 17, 2018
@ElvinEfendi
Copy link
Member

@fntlnz ingress-nginx already has /metrics endpoint where it exposes Prometheus metrics. I wonder why did you not use https://github.com/influxdata/telegraf/tree/release-1.6/plugins/inputs/prometheus to collect those metrics and push them to InfluxDB using https://github.com/influxdata/telegraf/tree/release-1.6/plugins/outputs/influxdb?

@fntlnz
Copy link
Contributor Author

fntlnz commented May 17, 2018

@ElvinEfendi that's a very different use case, the /metrics endpoint exposes collected and aggregated metrics related to the state of the ingress and the requests.
This on the other hand for every request sends a payload via udp to an InfluxDB telling some information about the request.

Worth noticing that you can also use telegraf instead of influxdb deployed as sidecar or at any other boundary if you don't want to send data to influxdb directly or you want to send the requests in batch of 10s 100s 1000s out of the node/network segment.

So the main differences are:

  • This doesn't do any aggregation on the host, just an udp call
  • This pushes the specific request infromation instead of pulling a summary of multiple requests

In the usecase we built it for it does not replace the /metrics endpoint, it is complimentary to it by giving access to the specific point in time which is the request.

Signed-off-by: Lorenzo Fontana <lo@linux.com>
Signed-off-by: Lorenzo Fontana <lo@linux.com>
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 19, 2018
@fntlnz fntlnz force-pushed the nginx-module branch 2 times, most recently from 1dc1c58 to 7e62135 Compare May 19, 2018 15:28
@fntlnz
Copy link
Contributor Author

fntlnz commented May 19, 2018

@aledbf pushed the e2e tests, this is now ready for review.

Signed-off-by: Lorenzo Fontana <lo@linux.com>
@aledbf
Copy link
Member

aledbf commented May 21, 2018

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2018
@aledbf
Copy link
Member

aledbf commented May 21, 2018

@fntlnz thanks!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, fntlnz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 21, 2018
@k8s-ci-robot k8s-ci-robot merged commit 5249610 into kubernetes:master May 21, 2018
@fntlnz fntlnz deleted the nginx-module branch May 21, 2018 21:31
@ElvinEfendi
Copy link
Member

@fntlnz thanks for the detailed use-case explanation - that makes sense!

@kfox1111
Copy link

Should the annotation be called something other then influxdb if that is the main use case? I could have easily overlooked it as I don't use influxdb.

@fntlnz
Copy link
Contributor Author

fntlnz commented May 29, 2018

@kfox1111 I don't understand what do you mean, can you give some examples?

@kfox1111
Copy link

@fntlnz the annotations are:
+|nginx.ingress.kubernetes.io/enable-influxdb|"true" or "false"|
+|nginx.ingress.kubernetes.io/influxdb-measurement|string|
+|nginx.ingress.kubernetes.io/influxdb-port|string|
+|nginx.ingress.kubernetes.io/influxdb-host|string|
+|nginx.ingress.kubernetes.io/influxdb-server-name|string|

But if you were not interested in using influxdb, but interested in "using udp to gather request statistics", its not obvious that you should be using the influx settings for that.

@aledbf
Copy link
Member

aledbf commented May 29, 2018

@kfox1111 those settings ARE for influxdb. There is no "generic" nginx module for using UDP to gather request statistics, you need an specific module for that

@kfox1111
Copy link

But in an earlier comment, you said you could send it to telegraph instead of influxdb. so its not really an influxdb thing specifically?

@aledbf
Copy link
Member

aledbf commented May 29, 2018

But in an earlier comment, you said you could send it to telegraph instead of influxdb. so its not really an influxdb thing specifically?

Yes, you can do that through an influxdb output plugin but this is done in influxdb, not nginx itself

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants