Metrics policy #860

davidor · 2018-08-29T13:13:23Z

Closes (partially) #745

This is an initial version with a reduced number of metrics. For now, it includes:

3scale backend response status codes.
connections (accepted, active, handled, reading, total, waiting, writing).
nginx errors.
capacity of every shared dictionary.
free space of every shared dictionary.

This is an example output:

# HELP backend_response Response status codes from 3scale's backend
# TYPE backend_response counter
backend_response{status="2xx"} 1
# HELP nginx_http_connections Number of HTTP connections
# TYPE nginx_http_connections gauge
nginx_http_connections{state="accepted"} 3
nginx_http_connections{state="active"} 1
nginx_http_connections{state="handled"} 3
nginx_http_connections{state="reading"} 0
nginx_http_connections{state="total"} 3
nginx_http_connections{state="waiting"} 0
nginx_http_connections{state="writing"} 1
# HELP nginx_metric_errors_total Number of nginx-lua-prometheus errors
# TYPE nginx_metric_errors_total counter
nginx_metric_errors_total 0
# HELP openresty_shdict_capacity OpenResty shared dictionary capacity
# TYPE openresty_shdict_capacity gauge
openresty_shdict_capacity{dict="api_keys"} 10485760
openresty_shdict_capacity{dict="batched_reports"} 1048576
openresty_shdict_capacity{dict="batched_reports_locks"} 1048576
openresty_shdict_capacity{dict="cached_auths"} 1048576
openresty_shdict_capacity{dict="configuration"} 10485760
openresty_shdict_capacity{dict="init"} 16384
openresty_shdict_capacity{dict="limiter"} 1048576
openresty_shdict_capacity{dict="locks"} 1048576
openresty_shdict_capacity{dict="prometheus_metrics"} 16777216
# HELP openresty_shdict_free_space OpenResty shared dictionary free space
# TYPE openresty_shdict_free_space gauge
openresty_shdict_free_space{dict="api_keys"} 10412032
openresty_shdict_free_space{dict="batched_reports"} 1032192
openresty_shdict_free_space{dict="batched_reports_locks"} 1032192
openresty_shdict_free_space{dict="cached_auths"} 1032192
openresty_shdict_free_space{dict="configuration"} 10412032
openresty_shdict_free_space{dict="init"} 4096
openresty_shdict_free_space{dict="limiter"} 1032192
openresty_shdict_free_space{dict="locks"} 1032192
openresty_shdict_free_space{dict="prometheus_metrics"} 16662528

codeclimate · 2018-08-29T13:14:14Z

gateway/src/apicast/policy/metrics/metrics.lua

+end
+
+local function filter_level()
+  local filter_level = resty_env.value(log_level_env) or log_level_default


shadowing upvalue 'filter_level' on line 42

codeclimate · 2018-08-29T14:57:32Z

gateway/src/apicast/policy/metrics/metrics.lua

+end
+
+local function filter_level()
+  local filter_level = resty_env.value(log_level_env) or log_level_default


shadowing upvalue 'filter_level' on line 43

mikz · 2018-08-30T07:48:26Z

gateway/src/apicast/metrics/3scale_backend_status.lua

+)
+
+function _M.inc(status)
+  if status >= 200 and status < 300 then


Wouldn't be better to use math ?

string.format('%dxx', (404/100))

And handle nil and 0 separately.

mikz · 2018-08-30T07:50:11Z

gateway/src/apicast/policy/metrics/metrics.lua

+
+local new = _M.new
+
+local log_map = {


This should be log_list? map would mean a hash table?

mikz · 2018-08-30T07:50:35Z

gateway/src/apicast/policy/metrics/metrics.lua

+  'debug',
+}
+
+local log_level_env = 'METRICS_LOG_LEVEL'


Maybe we should prefix it with APICAST_ ?

Maybe it's better to name it NGINX_METRICS_LOG_LEVEL as per your comment below about the name of the policy.

mikz · 2018-08-30T07:51:14Z

gateway/src/apicast/policy/metrics/metrics.lua

+local log_level_default = 'error'
+local max_logs_default = 100
+
+local function find_i(t, value)


Now when thinking about it. It might be good to hash this as value => i so we have constant time lookup.

Yes, that'd be better for when we want the index, but there's also https://github.com/3scale/apicast/blob/f371e5e4ec18c5046175eb2abc07d7b1c6928e7e/gateway/src/apicast/policy/metrics/metrics.lua#L88 below, so looks like we need both (value=>i, i=>value)

Yep, thats fine no? Just need to process it once.

Right now it only iterates through all the elements (and there are only 8 of them) in init().
In metrics() it's fetching an element from the list given an index, so this is fine as it is now. I changed the name of the var as you suggested above.

mikz · 2018-08-30T07:52:54Z

gateway/src/apicast/policy/metrics/metrics.lua

@@ -0,0 +1,113 @@
+local _M = require('apicast.policy').new('Metrics')


We might want to call this Nginx Metrics ? Probably need some input from @3scale/product.

Several policies will be exposing metrics and this one is really focused on internal nginx stuff like shared dictionaries, connections etc.

I agree. I think we can go ahead and change it.
This is a policy included by default that does not have a JSON manifest. That means that the name will not be visible anywhere in the UI.

mikz · 2018-08-30T07:54:01Z

gateway/src/apicast/policy/metrics/metrics.lua

+function _M:metrics()
+  local logs = get_logs(self.max_logs)
+
+  for i = 1, #logs, 3 do


Each of those 3 different metrics could be own function to reduce complexity.

Or maybe extract them to their own module like this one: https://github.com/3scale/apicast/pull/860/files#diff-0a1eff24f8903a000e877b8fedece766

I'd rather address refactorings in a future PR.

This has been imported from https://github.com/3scale/apicast-cloud-hosted/

This is needed to be able to use ngx.errlogs methods that we call from the Metrics policy. Ref: https://github.com/openresty/lua-nginx-module#lua_capture_error_log

The Executor calls policies .init() without self

davidor · 2018-08-30T13:18:55Z

@mikz I addressed all the comments except one #860 (comment) which I'd rather address separately.
I've also added an integration test for the backend response metrics: c70096d

mikz · 2018-08-30T13:25:11Z

gateway/src/apicast/metrics/3scale_backend_status.lua

+
+function _M.inc(status)
+  if not status or status == 0 then
+    ngx.log(ngx.WARN, 'Invalid status received')


Isn't it better to actually measure this ? It would be interesting metric no?

Good point. I think the http client might return 0 in some error cases, for example. Would be interesting to monitor those as well.

mikz

I think it is fine 👍

I'd really consider measuring also the invalid responses from 3scale backend. Those are even more interesting than 2xx and 4xx because you actually want to have alerts when that happens.

And it would be good to have a list of metrics and an example in the PR description.

davidor · 2018-08-30T13:42:16Z

I added the list of metrics and an example in the PR description.

davidor requested a review from a team as a code owner August 29, 2018 13:13

codeclimate bot reviewed Aug 29, 2018

View reviewed changes

davidor force-pushed the metrics-policy branch from ca213ab to f371e5e Compare August 29, 2018 14:59

mikz reviewed Aug 30, 2018

View reviewed changes

davidor force-pushed the metrics-policy branch from f371e5e to e929942 Compare August 30, 2018 09:12

davidor added 7 commits August 30, 2018 11:14

Add nginx metrics policy

9c004bc

This has been imported from https://github.com/3scale/apicast-cloud-hosted/

http.d: add conf file that sets lua_capture_error_log

418c3a4

This is needed to be able to use ngx.errlogs methods that we call from the Metrics policy. Ref: https://github.com/openresty/lua-nginx-module#lua_capture_error_log

config/{production,staging}: add metric ports

1012e17

policy_chain: include the nginx metrics policy by default

023ea0f

policy/nginx_metrics: modify init() so it does not depend on self

ee969bf

The Executor calls policies .init() without self

t: add tests for the prometheus metrics endpoint

3ae12ce

Extract common metrics functionality to a new module

6f25e5d

davidor force-pushed the metrics-policy branch from e929942 to 2167081 Compare August 30, 2018 09:20

davidor changed the title ~~[WIP] Metrics policy~~ Metrics policy Aug 30, 2018

mikz reviewed Aug 30, 2018

View reviewed changes

mikz approved these changes Aug 30, 2018

View reviewed changes

davidor added 3 commits August 30, 2018 15:41

metrics: create 3scale backend status

23b17c9

backend_client: update 3scale backend status metric

9b8454e

t/prometheus-metrics: test backend response metrics

9661f32

davidor force-pushed the metrics-policy branch from c70096d to 9661f32 Compare August 30, 2018 13:41

CHANGELOG: add metrics policy

77cf346

davidor merged commit c27e7f5 into master Aug 30, 2018

davidor deleted the metrics-policy branch August 30, 2018 14:13

davidor mentioned this pull request Aug 31, 2018

Basic prometheus metrics #745

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metrics policy #860

Metrics policy #860

davidor commented Aug 29, 2018 •

edited

Loading

codeclimate bot Aug 29, 2018

codeclimate bot Aug 29, 2018

mikz Aug 30, 2018

davidor Aug 30, 2018

mikz Aug 30, 2018

mikz Aug 30, 2018

davidor Aug 30, 2018

mikz Aug 30, 2018

davidor Aug 30, 2018

mikz Aug 30, 2018

davidor Aug 30, 2018

mikz Aug 30, 2018

davidor Aug 30, 2018

mikz Aug 30, 2018

davidor Aug 30, 2018

davidor Aug 30, 2018

mikz Aug 30, 2018

davidor commented Aug 30, 2018

mikz Aug 30, 2018

davidor Aug 30, 2018

davidor Aug 30, 2018

mikz left a comment

davidor commented Aug 30, 2018

		@@ -0,0 +1,113 @@
		local _M = require('apicast.policy').new('Metrics')

Metrics policy #860

Metrics policy #860

Conversation

davidor commented Aug 29, 2018 • edited Loading

codeclimate bot Aug 29, 2018

Choose a reason for hiding this comment

codeclimate bot Aug 29, 2018

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

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

davidor commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikz left a comment

Choose a reason for hiding this comment

davidor commented Aug 30, 2018

davidor commented Aug 29, 2018 •

edited

Loading