Add metric emitter lua module #2611

fmejia97 · 2018-06-06T17:52:31Z

What this PR does / why we need it:
This PR adds the metric_emitter lua module that emits UDP messages with NGINX stats to the controller.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

aledbf · 2018-06-06T18:07:22Z

@fmejia97 please check the lint errors https://travis-ci.org/kubernetes/ingress-nginx/jobs/388896002#L721

aledbf · 2018-06-06T18:20:22Z

rootfs/etc/nginx/lua/metric_emitter.lua

+
+  table.insert(_M.queue, jsonPayload)
+
+  local ok, err = ngx.timer.at(0, flush_queue)


what's the behavior of ngx.timer.at when nginx is receiving more than 1024 rps? (the default for pending timers)
https://github.com/openresty/lua-nginx-module#lua_max_pending_timers

It will return nil, together with an error (from the documentation you linked)

@aledbf we chatted about this, we'll be addressing it. Great catch!

We need to follow the way we did it in our fork - where we essentially queue up request logs and pop it all off at a timer event that runs every second.

@fmejia97 will be addressing that today :)

@aledbf Also worth mentioning - we use a nginx module to expose the active number of running timers; possibly worth merging this upstream to https://github.com/openresty/lua-nginx-module? Wdyt?

Wdyt?

That sounds like a good idea 👍

@fmejia97 @andrewlouis93 why don't you fork lift defer.lua from our fork and just do defer.to_timer_phase(<you function that sends the json payload>) in this middleware

Just did :) @ElvinEfendi

codecov-io · 2018-06-06T19:39:59Z

Codecov Report

Merging #2611 into master will decrease coverage by 0.13%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2611      +/-   ##
==========================================
- Coverage   40.83%   40.69%   -0.14%     
==========================================
  Files          75       75              
  Lines        5123     5123              
==========================================
- Hits         2092     2085       -7     
- Misses       2750     2756       +6     
- Partials      281      282       +1

Impacted Files	Coverage Δ
cmd/nginx/main.go	`23.66% <0%> (-5.35%)`	⬇️

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 764bcd5...966e9f5. Read the comment docs.

ElvinEfendi · 2018-06-07T23:50:47Z

rootfs/etc/nginx/lua/metric_emitter.lua

+end
+
+function _M.call()
+  -- Create JSON Metrics Payload  --


no need to explain the obvious

ElvinEfendi · 2018-06-08T00:11:08Z

rootfs/etc/nginx/lua/metric_emitter.lua

+    upstreamStatus       = ngx.var.upstream_status                  or "-",
+    namespace            = ngx.var.namespace                        or "-",
+    ingress              = ngx.var.ingress_name                     or "-",
+    service              = ngx.var.service_name                     or "-",


can you avoid spacing like this? later on when we add another line that's longer we then will have to adjust all the others

Where does this list come from? From VTS?

The stats on the list were given by @aledbf on the kubernetes slack channel. This does not yet cover all VTS metrics.

@fmejia97 feel free to add all the fields you think are required to match the VTS module. I just only added the minimum to start the work with the UDP server

ElvinEfendi · 2018-06-08T00:12:52Z

rootfs/etc/nginx/lua/metric_emitter.lua

+local assert = assert
+
+local _M = {
+  queue = {}


queue can be local, no? why does it have to be exported?

This is not needed anymore since we are going to use the defer module to delegate deferring callbacks (with queue optimization) to the timer phase.

@fmejia97 could you use this version of the module instead: https://github.com/Shopify/ingress/blob/master/rootfs/etc/nginx/lua/util/defer.lua?

This doesn't export things that don't need to be exported

ElvinEfendi · 2018-06-08T00:24:38Z

rootfs/etc/nginx/lua/metric_emitter.lua

What about monitor.lua?

ElvinEfendi · 2018-06-08T00:31:44Z

rootfs/etc/nginx/lua/metric_emitter.lua

+  local jsonPayload = cjson.encode({
+    host                 = ngx.var.host                             or "-",
+    status               = ngx.var.status                           or "-",
+    remoteAddr           = ngx.var.realip_remote_addr               or "-",


This is a bit confusing. ngx.var.realip_remote_addr is the original client IP, but remoteAddr indicates client IP (obtained from X-Frowarded-For if set). I'd say ngx.var.remote_addr is more valuable since it will be the true client IP.

Maybe have both?

Good point. I'll add both to avoid confusion.

ElvinEfendi · 2018-06-08T00:33:42Z

rootfs/etc/nginx/lua/metric_emitter.lua

+    bytesSent            = tonumber(ngx.var.bytes_sent)             or -1,
+    protocol             = ngx.var.server_protocol                  or "-",
+    method               = ngx.var.request_method                   or "-",
+    path                 = ngx.var.uri                              or "-",


why not key it as uri instead of path to have better clarity and consistency with the rest of the mappings?

ElvinEfendi · 2018-06-08T00:34:26Z

rootfs/etc/nginx/lua/metric_emitter.lua

+    method               = ngx.var.request_method                   or "-",
+    path                 = ngx.var.uri                              or "-",
+    requestLength        = tonumber(ngx.var.request_length)         or -1,
+    requestDuration      = tonumber(ngx.var.request_time)           or -1,


same as above, requestTime

ElvinEfendi · 2018-06-08T00:35:50Z

rootfs/etc/nginx/lua/metric_emitter.lua

+    path                 = ngx.var.uri                              or "-",
+    requestLength        = tonumber(ngx.var.request_length)         or -1,
+    requestDuration      = tonumber(ngx.var.request_time)           or -1,
+    upstreamName         = ngx.var.upstream                         or "-",


ngx.var.upstream does not seem like a standard Nginx variable, are we setting it ourselves somewhere?

Good catch, that was a mistake. It should've been ngx.var.proxy_upstream_name.

ElvinEfendi · 2018-06-08T00:37:34Z

rootfs/etc/nginx/lua/metric_emitter.lua

+    requestDuration      = tonumber(ngx.var.request_time)           or -1,
+    upstreamName         = ngx.var.upstream                         or "-",
+    upstreamIP           = ngx.var.upstream_addr                    or "-",
+    upstreamResponseTime = tonumber(ngx.var.upstream_response_time) or -1,


How is this gonna work? when would tonumber return nil?

It returns nil when it fails to convert its argument to a number.

nil or -1 will return -1. The UDP listener expects some fields to be of type float64, and it will error when calling Unmarshal if the value of the field is not a number.

fmejia97 · 2018-06-08T04:30:40Z

@ElvinEfendi @andrewlouis93 Addressed your comments 👍

andrewloux · 2018-06-08T17:52:13Z

@fmejia97 We're still not matching all the fields published by the VTS module. I think this is the last thing we need to do before I think this can be merged in.

ElvinEfendi · 2018-06-11T12:23:19Z

Code looks good to me! Can you add unit test for monitor.call? After that this is ready to 🚢

Regarding to matching VTS metrics I have not checked that - I'll wait for @andrewlouis93 to approve it. But we can always revisit that part as long as we have the basics.

andrewloux · 2018-06-11T13:31:33Z

rootfs/etc/nginx/lua/defer.lua

+  return true
+end
+
+return _M


fmejia97 · 2018-06-11T15:48:24Z

@andrewlouis93 @aledbf @ElvinEfendi Added metrics for connections(active|reading|writing|waiting) and total requests.

andrewloux · 2018-06-11T19:00:11Z

Nice work @fmejia97 just some quick unit tests left now :)

fmejia97 · 2018-06-12T04:51:36Z

Added unit tests for monitor.lua and defer.lua. @ElvinEfendi @andrewlouis93 @aledbf

ElvinEfendi · 2018-06-12T14:24:08Z

rootfs/etc/nginx/lua/test/monitor/monitor_test.lua

+    get_phase = function() return "timer" end,
+    var = {}
+}
+  _G.ngx = _ngx


redundant indentation

ElvinEfendi · 2018-06-12T14:24:39Z

rootfs/etc/nginx/lua/test/monitor/monitor_test.lua

+describe("Monitor", function()
+    local monitor = require("monitor")
+    describe("encode_nginx_stats()", function()
+        ngx.var = 


what does this do?

@ElvinEfendi encode_nginx_stats() creates a JSON encoded structure using the current NGNX context stats (from ngx.var). In this test case, I'm setting a specific environment to test the method

🤔 I see you are doing it below but ngx.var = does not make sense

Ahh woops 😄didn't realize this. Removing 👍

ElvinEfendi · 2018-06-12T14:25:09Z

rootfs/etc/nginx/lua/test/monitor/monitor_test.lua

+    describe("encode_nginx_stats()", function()
+        ngx.var = 
+        it("successfuly encodes the current stats of nginx to JSON", function()
+            local nginxEnvironment = {


please use underscore naming convention when writing Lua code

ElvinEfendi · 2018-06-12T14:28:06Z

rootfs/etc/nginx/lua/test/defer/defer_test.lua

+describe("Defer", function()
+   describe("to_timer_phase", function()
+    it("executes passed callback immediately if called on timer phase", function()
+        assert.equal(true, defer.to_timer_phase(function() return 1 end))


how does this assert that the function was executed?

👍 Create a table here - and have the callback modify the table's value. E.g. Initialize defer.call_count = 0, and then in the callback - increment that value by 1.

Your assertion can check for that value having been increased.

The problem with this right now is that your test will pass regardless of whether or not the callback passed in to to_timer_phase is executed.

ElvinEfendi · 2018-06-12T14:29:15Z

why are you putting the tests into defer/ and monitor/ folders?

ElvinEfendi · 2018-06-12T14:30:53Z

rootfs/etc/nginx/lua/monitor.lua

+  assert(s:close())
+end
+
+function _M.encode_nginx_stats()


if you test the public .call method with proper assertions this will be covered as well and you won't need to export this method just for testing purposes.

You can export only if you justify that testing .call is unnecessarily complex.

@ElvinEfendi My initial approach was to test .call method. Yesterday I spent a fair bit of the day trying to figure out how to make it work but couldn't find a clean / non-hacky way of testing it. Mostly due to the nature of the module: 1) Generate JSON struct form nginx context variables. 2) Create a timer with send_data callback. 3) Send the JSON struct as a UDP message. The main logic of the module is building the JSON structure (then use the interface of the other modules which are already tested). After pairing with Andrew we decided that this would be the cleanest way of going forward.

andrewloux · 2018-06-12T14:52:09Z

rootfs/etc/nginx/lua/test/monitor/monitor_test.lua

+                service = "test-app",
+            }
+
+            assert.are.same(decodedJSONStats,expectedJSONStats)


this is some nice syntactic sugar 🍬

aledbf · 2018-06-13T20:43:03Z

@fmejia97 @andrewlouis93 @ElvinEfendi what's missing to merge this?

andrewloux

🚀 🚀 🚀

andrewloux · 2018-06-14T00:03:59Z

@aledbf this looks good, let's ship this.

aledbf · 2018-06-14T00:08:12Z

@fmejia97 please squash this in fewer commits and we are ready to go

fmejia97 · 2018-06-14T02:55:33Z

@aledbf Squashed commits ✅

aledbf · 2018-06-14T02:56:27Z

/lgtm
/approve

k8s-ci-robot · 2018-06-14T02:56:35Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [aledbf]

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 added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 6, 2018

aledbf assigned ElvinEfendi Jun 6, 2018

fmejia97 force-pushed the add-metric-emitter-module branch from a735193 to d8398b4 Compare June 6, 2018 18:19

aledbf reviewed Jun 6, 2018

View reviewed changes

fmejia97 force-pushed the add-metric-emitter-module branch from d8398b4 to e08c118 Compare June 6, 2018 18:58

fmejia97 mentioned this pull request Jun 6, 2018

Expose UDP message on /metrics endpoint #2608

Merged

ElvinEfendi reviewed Jun 7, 2018

View reviewed changes

ElvinEfendi reviewed Jun 8, 2018

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 8, 2018

fmejia97 force-pushed the add-metric-emitter-module branch from b9e8f43 to 6c282df Compare June 8, 2018 14:28

andrewloux reviewed Jun 11, 2018

View reviewed changes

rootfs/etc/nginx/lua/defer.lua

return true

end

return _M

Copy link

Contributor

andrewloux Jun 11, 2018

Choose a reason for hiding this comment

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

Newline

fmejia97 reacted with thumbs up emoji

fmejia97 force-pushed the add-metric-emitter-module branch 2 times, most recently from 571ee57 to a97baf9 Compare June 12, 2018 04:54

ElvinEfendi reviewed Jun 12, 2018

View reviewed changes

andrewloux reviewed Jun 12, 2018

View reviewed changes

andrewloux approved these changes Jun 13, 2018

View reviewed changes

Add monitor lua module

966e9f5

fmejia97 force-pushed the add-metric-emitter-module branch from dc43bdf to 966e9f5 Compare June 14, 2018 02:54

k8s-ci-robot assigned aledbf Jun 14, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 14, 2018

k8s-ci-robot merged commit 43cf56c into kubernetes:master Jun 14, 2018


		table.insert(_M.queue, jsonPayload)

		local ok, err = ngx.timer.at(0, flush_queue)

Add metric emitter lua module #2611

Add metric emitter lua module #2611

Conversation

fmejia97 commented Jun 6, 2018

aledbf commented Jun 6, 2018

aledbf Jun 6, 2018 • 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

aledbf Jun 7, 2018 • edited Loading

Choose a reason for hiding this comment

ElvinEfendi Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 6, 2018 • edited Loading

Codecov Report

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

ElvinEfendi commented Jun 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmejia97 Jun 8, 2018 • 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

fmejia97 commented Jun 8, 2018 • edited Loading

andrewloux commented Jun 8, 2018

ElvinEfendi commented Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

fmejia97 commented Jun 11, 2018

andrewloux commented Jun 11, 2018

fmejia97 commented Jun 12, 2018 • 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

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

ElvinEfendi commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aledbf commented Jun 13, 2018

andrewloux left a comment

Choose a reason for hiding this comment

andrewloux commented Jun 14, 2018

aledbf commented Jun 14, 2018

fmejia97 commented Jun 14, 2018

aledbf commented Jun 14, 2018

k8s-ci-robot commented Jun 14, 2018

aledbf Jun 6, 2018 •

edited

Loading

aledbf Jun 7, 2018 •

edited

Loading

ElvinEfendi Jun 8, 2018 •

edited

Loading

codecov-io commented Jun 6, 2018 •

edited

Loading

ElvinEfendi Jun 8, 2018 •

edited

Loading

fmejia97 Jun 8, 2018 •

edited

Loading

fmejia97 commented Jun 8, 2018 •

edited

Loading

ElvinEfendi commented Jun 11, 2018 •

edited

Loading

fmejia97 commented Jun 12, 2018 •

edited

Loading