feat(prometheus) update to the new kong db #24
Conversation
kikito
force-pushed
the
feat/update-to-new-db
branch
from
dbbb6c3
to
9a7aaa0
Compare
|
Thank you @kikito for taking care of this transition! |
| @@ -17,7 +12,7 @@ local prometheus | |||
| local function init() | |||
| local shm = "prometheus_metrics" | |||
| if not ngx.shared.prometheus_metrics then | |||
| ngx_log(ERR, "prometheus: ngx shared dict 'prometheus_metrics' not found") | |||
| kong.log.ERR("prometheus: ngx shared dict 'prometheus_metrics' not found") | |||
There was a problem hiding this comment.
kong.log.ERR is not a function, this will attempt to call nil. kong.log.err is the correct name. Was this change tested?
There was a problem hiding this comment.
There is no test for this change and is a difficult one to test since this condition doesn't occur in in Kong >= 0.14.0 as Kong injects the shm whenever it detects prometheus plugin is loaded at startup.
There was a problem hiding this comment.
By "this change", I am referring to this entire diff (any of the error logs produced in any of the unhappy paths touched by this patch). I already assume that the answer is no. What will we do to fix this patch and ship another release?
There was a problem hiding this comment.
You probably know but just want to note that the diff was tested against next branch of Kong's Git repo.
To fix this patch, I'll make another PR to fix the kong.log.* functions and will try to come up with tests for unhappy path on Line 85 of this file but I don't know how to test other unhappy paths.
Other unhappy paths are rare conditions that are hard to simulate, but happy to write those tests if it's possible to do those. Any pointers that I could refer?
There was a problem hiding this comment.
To fix this patch, I'll make another PR to fix the
kong.log.*functions
Sweet!
I don't know how to test other unhappy paths.
We don't test all unhappy paths (especially when they are tricky to catch). They often make a large part of the uncovered portion of many projects we develop or work with. Simply commenting out or tweaking an unhappy condition (like commenting out the surrounding branch) could also tell us if kong.log.ERR is offered as an alias to kong.log.err (spoiler alert: it isn't). Of course, if they aren't already, testing unhappy paths as part of CI would set us up a million times better off 👍
Other unhappy paths are rare conditions that are hard to simulate, but happy to write those tests if it's possible to do those. Any pointers that I could refer?
I have tested race conditions with various tricks in the past (ngx.thread/sleep/semaphore/etc...) but they are probably not worth the time for such high-level utilities like this plugin, unless you have lots of time on your hands :)
| return responses.send_HTTP_INTERNAL_SERVER_ERROR() | ||
| end | ||
|
|
||
| local r = ngx.location.capture "/nginx_status" | ||
|
|
||
| if r.status ~= 200 then | ||
| ngx_log(WARN, "prometheus: failed to retrieve /nginx_status ", | ||
| "while processing /metrics endpoint") | ||
| kong.log.WARN("prometheus: failed to retrieve /nginx_status ", |
| GET = function(self, dao_factory) -- luacheck: ignore 212 | ||
| prometheus.collect() | ||
| end, | ||
| schema = consumers_schema, -- not used, could be any schema |
There was a problem hiding this comment.
This comment is confusing... The reader is left wondering why we are specifying a schema at all then?
There was a problem hiding this comment.
Is there a null schema in Kong? Also, is this a required field (doesn't seem as we can put anything in there)?
This PR changes the plugin to make it compatible with the current next branch in kong.
prometheus/prometheus.luavendored dependency has been left untouched.