diff --git a/CHANGELOG.md b/CHANGELOG.md index eff65336c..30e8afeb0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,11 +10,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Prometheus metrics for: the 3scale batching policy and the upstream API [PR #902](https://github.com/3scale/apicast/pull/902), [PR #918](https://github.com/3scale/apicast/pull/918) - Support for path in the upstream URL [PR #905](https://github.com/3scale/apicast/pull/905) -- OIDC Authentication policy (only useable directly by the configuration file) [PR #904](https://github.com/3scale/apicast/pull/904) +- OIDC Authentication policy (only usable directly by the configuration file) [PR #904](https://github.com/3scale/apicast/pull/904) ### Changed -- Renamed the `backend_response` Prometheus metric to `threescale_backend_response` to avoid confusion with the upstream response [PR #917](https://github.com/3scale/apicast/pull/917) +- The `threescale_backend_calls` Prometheus metric now includes the response (used to be in `backend_response`) and also the kind of call (auth, authrep, report)[PR #919](https://github.com/3scale/apicast/pull/919) ## [3.3.0-cr2] - 2018-09-25 diff --git a/gateway/src/apicast/backend_client.lua b/gateway/src/apicast/backend_client.lua index e7439940b..12603e0cb 100644 --- a/gateway/src/apicast/backend_client.lua +++ b/gateway/src/apicast/backend_client.lua @@ -20,7 +20,7 @@ local http_ng = require('resty.http_ng') local user_agent = require('apicast.user_agent') local resty_url = require('resty.url') local resty_env = require('resty.env') -local threescale_backend_status_counters = require('apicast.metrics.3scale_backend_status') +local backend_calls_metrics = require('apicast.metrics.3scale_backend_calls') local http_proxy = require('resty.http.proxy') local http_ng_ngx = require('resty.http_ng.backend.ngx') @@ -98,8 +98,8 @@ function _M:new(service, http_client) }, mt) end -local function inc_backend_status_metric(status) - threescale_backend_status_counters.inc(status) +local function inc_metrics(endpoint, status) + backend_calls_metrics.report(endpoint, status) end local function build_args(args) @@ -138,8 +138,6 @@ local function call_backend_transaction(self, path, options, ...) ngx.log(ngx.INFO, 'backend client uri: ', url, ' ok: ', res.ok, ' status: ', res.status, ' body: ', res.body, ' error: ', res.error) - inc_backend_status_metric(res.status) - return res end @@ -213,7 +211,11 @@ function _M:authrep(...) local using_oauth = self.version == 'oauth' local auth_uri = authrep_path(using_oauth) - return call_backend_transaction(self, auth_uri, authorize_options(using_oauth), ...) + local res = call_backend_transaction(self, auth_uri, authorize_options(using_oauth), ...) + + inc_metrics('authrep', res.status) + + return res end --- Call authorize (oauth_authorize) on backend. @@ -226,7 +228,11 @@ function _M:authorize(...) local using_oauth = self.version == 'oauth' local auth_uri = auth_path(using_oauth) - return call_backend_transaction(self, auth_uri, authorize_options(using_oauth), ...) + local res = call_backend_transaction(self, auth_uri, authorize_options(using_oauth), ...) + + inc_metrics('auth', res.status) + + return res end function _M:report(reports_batch) @@ -236,7 +242,7 @@ function _M:report(reports_batch) local report_body = format_transactions(reports_batch) local res = http_client.post(report_uri, report_body) - inc_backend_status_metric(res.status) + inc_metrics('report', res.status) return res end diff --git a/gateway/src/apicast/metrics/3scale_backend_calls.lua b/gateway/src/apicast/metrics/3scale_backend_calls.lua new file mode 100644 index 000000000..44115e0d2 --- /dev/null +++ b/gateway/src/apicast/metrics/3scale_backend_calls.lua @@ -0,0 +1,28 @@ +local prometheus = require('apicast.prometheus') + +local format = string.format + +local _M = {} + +local threescale_backend_call = prometheus( + 'counter', + 'threescale_backend_calls', + "Calls to the 3scale backend", + { 'endpoint', 'status' } +) + +local function label_for_status(status) + if not status or status == '' or status == 0 then + return 'invalid_status' + else + return format("%dxx", status/100) + end +end + +function _M.report(endpoint, status) + if threescale_backend_call then + threescale_backend_call:inc(1, { endpoint, label_for_status(status) }) + end +end + +return _M diff --git a/gateway/src/apicast/metrics/3scale_backend_status.lua b/gateway/src/apicast/metrics/3scale_backend_status.lua deleted file mode 100644 index 4613b957a..000000000 --- a/gateway/src/apicast/metrics/3scale_backend_status.lua +++ /dev/null @@ -1,27 +0,0 @@ -local prometheus = require('apicast.prometheus') -local metrics_updater = require('apicast.metrics.updater') - -local format = string.format - -local _M = {} - -local backend_response_metric = prometheus( - 'counter', - 'threescale_backend_response', - "Response status codes from 3scale's backend", - { 'status' } -) - -local function label_for_status(status) - if not status or status == 0 then - return 'invalid_status' - else - return format("%dxx", status/100) - end -end - -function _M.inc(status) - metrics_updater.inc(backend_response_metric, label_for_status(status)) -end - -return _M diff --git a/spec/backend_client_spec.lua b/spec/backend_client_spec.lua index 978729776..9d143e2c0 100644 --- a/spec/backend_client_spec.lua +++ b/spec/backend_client_spec.lua @@ -1,7 +1,8 @@ local _M = require('apicast.backend_client') local configuration = require('apicast.configuration') -local test_backend_client = require 'resty.http_ng.backend.test' +local http_ng = require 'resty.http_ng' local ReportsBatch = require 'apicast.policy.3scale_batcher.reports_batch' +local backend_calls_metrics = require 'apicast.metrics.3scale_backend_calls' describe('backend client', function() @@ -9,7 +10,10 @@ describe('backend client', function() local options_header_oauth = 'rejection_reason_header=1' local options_header_no_oauth = 'rejection_reason_header=1&no_body=1' - before_each(function() test_backend = test_backend_client.new() end) + before_each(function() + test_backend = http_ng.backend() + stub(backend_calls_metrics, 'report') + end) describe('authrep', function() it('works without params', function() @@ -90,6 +94,17 @@ describe('backend client', function() assert.equal(200, res.status) end) + + it('reports the call with the status', function() + local service = configuration.parse_service({ id = '42' }) + local status = 200 + test_backend.expect({}).respond_with({ status = status }) + local backend_client = assert(_M:new(service, test_backend)) + + backend_client:authrep() + + assert.stub(backend_calls_metrics.report).was_called_with('authrep', status) + end) end) describe('authorize', function() @@ -172,103 +187,125 @@ describe('backend client', function() assert.equal(200, res.status) end) - describe('report', function() - describe('when the service is configured to use app IDs', function() - it('makes the call to backend with the right params', function() - local service = configuration.parse_service({ - id = '42', - backend_version = '2', - proxy = { backend = { endpoint = 'http://example.com' } }, - backend_authentication_type = 'auth', backend_authentication_value = 'val' - }) - - -- It's tricky to test with several reports because they can go in - -- any order in the request. - local reports = { { app_id = 'id1', metric = 'm1', value = 1 } } - - local transactions = {} - transactions["transactions[0][app_id]"] = 'id1' - transactions["transactions[0][usage][m1]"] = 1 - - local reports_batch = ReportsBatch.new(service.id, reports) - - test_backend.expect{ - url = 'http://example.com/transactions.xml?' .. - ngx.encode_args({ auth = service.backend_authentication.value, - service_id = service.id }), - body = ngx.encode_args(transactions) - }.respond_with{ status = 200 } - - local backend_client = assert(_M:new(service, test_backend)) - local res = backend_client:report(reports_batch) - assert.equal(200, res.status) - end) + it('reports the call with the status', function() + local service = configuration.parse_service({ id = '42' }) + local status = 200 + test_backend.expect({}).respond_with({ status = status }) + local backend_client = assert(_M:new(service, test_backend)) + + backend_client:authorize() + + assert.stub(backend_calls_metrics.report).was_called_with('auth', status) + end) + end) + + describe('report', function() + describe('when the service is configured to use app IDs', function() + it('makes the call to backend with the right params', function() + local service = configuration.parse_service({ + id = '42', + backend_version = '2', + proxy = { backend = { endpoint = 'http://example.com' } }, + backend_authentication_type = 'auth', backend_authentication_value = 'val' + }) + + -- It's tricky to test with several reports because they can go in + -- any order in the request. + local reports = { { app_id = 'id1', metric = 'm1', value = 1 } } + + local transactions = {} + transactions["transactions[0][app_id]"] = 'id1' + transactions["transactions[0][usage][m1]"] = 1 + + local reports_batch = ReportsBatch.new(service.id, reports) + + test_backend.expect{ + url = 'http://example.com/transactions.xml?' .. + ngx.encode_args({ auth = service.backend_authentication.value, + service_id = service.id }), + body = ngx.encode_args(transactions) + }.respond_with{ status = 200 } + + local backend_client = assert(_M:new(service, test_backend)) + local res = backend_client:report(reports_batch) + assert.equal(200, res.status) end) + end) - describe('when the service is configured to use user keys', function() - it('makes the call to backend with the right params', function() - local service = configuration.parse_service({ - id = '42', - backend_version = '1', - proxy = { backend = { endpoint = 'http://example.com' } }, - backend_authentication_type = 'auth', backend_authentication_value = 'val' - }) - - -- It's tricky to test with several reports because they can go in - -- any order in the request. - local reports = { { user_key = 'uk1', metric = 'm1', value = 1 } } - - local transactions = {} - transactions["transactions[0][user_key]"] = 'uk1' - transactions["transactions[0][usage][m1]"] = 1 - - local reports_batch = ReportsBatch.new(service.id, reports) - - test_backend.expect{ - url = 'http://example.com/transactions.xml?' .. - ngx.encode_args({ auth = service.backend_authentication.value, - service_id = service.id }), - body = ngx.encode_args(transactions) - }.respond_with{ status = 200 } - - local backend_client = assert(_M:new(service, test_backend)) - local res = backend_client:report(reports_batch) - assert.equal(200, res.status) - end) + describe('when the service is configured to use user keys', function() + it('makes the call to backend with the right params', function() + local service = configuration.parse_service({ + id = '42', + backend_version = '1', + proxy = { backend = { endpoint = 'http://example.com' } }, + backend_authentication_type = 'auth', backend_authentication_value = 'val' + }) + + -- It's tricky to test with several reports because they can go in + -- any order in the request. + local reports = { { user_key = 'uk1', metric = 'm1', value = 1 } } + + local transactions = {} + transactions["transactions[0][user_key]"] = 'uk1' + transactions["transactions[0][usage][m1]"] = 1 + + local reports_batch = ReportsBatch.new(service.id, reports) + + test_backend.expect{ + url = 'http://example.com/transactions.xml?' .. + ngx.encode_args({ auth = service.backend_authentication.value, + service_id = service.id }), + body = ngx.encode_args(transactions) + }.respond_with{ status = 200 } + + local backend_client = assert(_M:new(service, test_backend)) + local res = backend_client:report(reports_batch) + assert.equal(200, res.status) end) + end) - describe('when the service is configured to use oauth tokens', function() - it('makes the call to backend with the right params', function() - local service = configuration.parse_service({ - id = '42', - backend_version = 'oauth', - proxy = { backend = { endpoint = 'http://example.com' } }, - backend_authentication_type = 'auth', backend_authentication_value = 'val' - }) - - -- It's tricky to test with several reports because they can go in - -- any order in the request. - local reports = { { access_token = 'token', metric = 'm1', value = 1 } } - - local transactions = {} - transactions["transactions[0][access_token]"] = 'token' - transactions["transactions[0][usage][m1]"] = 1 - - local reports_batch = ReportsBatch.new(service.id, reports) - - test_backend.expect{ - url = 'http://example.com/transactions.xml?' .. - ngx.encode_args({ auth = service.backend_authentication.value, - service_id = service.id }), - body = ngx.encode_args(transactions) - }.respond_with{ status = 200 } - - local backend_client = assert(_M:new(service, test_backend)) - local res = backend_client:report(reports_batch) - assert.equal(200, res.status) - end) + describe('when the service is configured to use oauth tokens', function() + it('makes the call to backend with the right params', function() + local service = configuration.parse_service({ + id = '42', + backend_version = 'oauth', + proxy = { backend = { endpoint = 'http://example.com' } }, + backend_authentication_type = 'auth', backend_authentication_value = 'val' + }) + + -- It's tricky to test with several reports because they can go in + -- any order in the request. + local reports = { { access_token = 'token', metric = 'm1', value = 1 } } + + local transactions = {} + transactions["transactions[0][access_token]"] = 'token' + transactions["transactions[0][usage][m1]"] = 1 + + local reports_batch = ReportsBatch.new(service.id, reports) + + test_backend.expect{ + url = 'http://example.com/transactions.xml?' .. + ngx.encode_args({ auth = service.backend_authentication.value, + service_id = service.id }), + body = ngx.encode_args(transactions) + }.respond_with{ status = 200 } + + local backend_client = assert(_M:new(service, test_backend)) + local res = backend_client:report(reports_batch) + assert.equal(200, res.status) end) end) + + it('reports the call with the status', function() + local service = configuration.parse_service({ id = '42' }) + local status = 200 + test_backend.expect({}).respond_with({ status = status }) + local backend_client = assert(_M:new(service, test_backend)) + + backend_client:report(ReportsBatch.new(service.id, {})) + + assert.stub(backend_calls_metrics.report).was_called_with('report', status) + end) end) describe('store_oauth_token', function() diff --git a/spec/metrics/3scale_backend_calls_spec.lua b/spec/metrics/3scale_backend_calls_spec.lua new file mode 100644 index 000000000..1f9722f46 --- /dev/null +++ b/spec/metrics/3scale_backend_calls_spec.lua @@ -0,0 +1,55 @@ +describe('backend calls metrics', function() + describe('report', function() + local threescale_backend_calls_metrics + local test_counter = { inc = function() end } + + before_each(function() + stub(test_counter, 'inc') + local Prometheus = require('apicast.prometheus') + getmetatable(Prometheus).__call = function(_, type) + if type == 'counter' then + return test_counter + end + end + + package.loaded['apicast.metrics.3scale_backend_calls'] = nil + threescale_backend_calls_metrics = require('apicast.metrics.3scale_backend_calls') + end) + + after_each(function() + package.loaded['apicast.prometheus'] = nil + require('apicast.prometheus') + + package.loaded['apicast.metrics.3scale_backend_calls'] = nil + require('apicast.metrics.3scale_backend_calls') + end) + + it('increases the counter for type of request and status code (2xx, 4xx, etc.)', function() + threescale_backend_calls_metrics.report('authrep', 200) + + assert.stub(test_counter.inc).was_called_with( + test_counter, 1, { 'authrep', '2xx' }) + end) + + it('sets the status code to "invalid_status" when it is nil', function() + threescale_backend_calls_metrics.report('auth', nil) + + assert.stub(test_counter.inc).was_called_with( + test_counter, 1, { 'auth', 'invalid_status' }) + end) + + it('set the status code to "invalid_status" when it is empty', function() + threescale_backend_calls_metrics.report('auth', '') + + assert.stub(test_counter.inc).was_called_with( + test_counter, 1, { 'auth', 'invalid_status' }) + end) + + it('sets the status code to "invalid_status" when it is 0', function() + threescale_backend_calls_metrics.report('auth', 0) + + assert.stub(test_counter.inc).was_called_with( + test_counter, 1, { 'auth', 'invalid_status' }) + end) + end) +end) diff --git a/t/prometheus-metrics.t b/t/prometheus-metrics.t index f45c616d5..f8cb6b888 100644 --- a/t/prometheus-metrics.t +++ b/t/prometheus-metrics.t @@ -111,10 +111,10 @@ that does not include the nginx metrics (tested in the previous test). # HELP nginx_metric_errors_total Number of nginx-lua-prometheus errors # TYPE nginx_metric_errors_total counter nginx_metric_errors_total 0 -# HELP threescale_backend_response Response status codes from 3scale's backend -# TYPE threescale_backend_response counter -threescale_backend_response{status="2xx"} 2 -threescale_backend_response{status="4xx"} 2 +# HELP threescale_backend_calls Calls to the 3scale backend +# TYPE threescale_backend_calls counter +threescale_backend_calls{endpoint="authrep",status="2xx"} 2 +threescale_backend_calls{endpoint="authrep",status="4xx"} 2 METRICS_OUTPUT ] --- no_error_log @@ -183,9 +183,9 @@ batching_policy_auths_cache_misses 1 # HELP nginx_metric_errors_total Number of nginx-lua-prometheus errors # TYPE nginx_metric_errors_total counter nginx_metric_errors_total 0 -# HELP threescale_backend_response Response status codes from 3scale's backend -# TYPE threescale_backend_response counter -threescale_backend_response{status="2xx"} 1 +# HELP threescale_backend_calls Calls to the 3scale backend +# TYPE threescale_backend_calls counter +threescale_backend_calls{endpoint="auth",status="2xx"} 1 METRICS_OUTPUT ] --- no_error_log