Metrics: add the type of 3scale backend call (auth, authrep, report) #919
Conversation
They were in the same 'describe' clause by mistake.
davidor
force-pushed
the
metrics-backend-calls-type
branch
from
7047f5e
to
ab04b0f
Compare
| if valid_endpoint[endpoint] then | ||
| threescale_backend_call:inc(1, { endpoint, label_for_status(status) }) | ||
| else | ||
| ngx.log(ngx.WARN, 'Invalid 3scale backend endpoint endpoint when updating metrics') |
There was a problem hiding this comment.
Do we really care about this?
edit: I think we don't need to handle this and can just record every endpoint. No code is better than no code.
There was a problem hiding this comment.
What about the possibility of having invalid information in Prometheus?
There was a problem hiding this comment.
But what would be that invalid information? Someone part of code calling the wrong endpoint on the backend. I don't consider that invalid metric. If that call happened, then it should be measured and reported.
spec/backend_client_spec.lua
Outdated
|
|
||
| describe('backend client', function() | ||
|
|
||
| local test_backend | ||
| 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 = test_backend_client.new() |
There was a problem hiding this comment.
This should be not necessary with the changes from 576fa67
spec/backend_client_spec.lua
Outdated
| it('reports the call with the status', function() | ||
| local service = configuration.parse_service({ id = '42' }) | ||
| local status = 200 | ||
| stub(test_backend, 'send', function() return { status = status } end) |
There was a problem hiding this comment.
Why this instead of the test_backend.expect ?
There was a problem hiding this comment.
Because in this case, we don't really care about what the backend client receives, we just want it to return a 200 so we can report it.
There was a problem hiding this comment.
Would something like test_backend.expect { }.respond_with{status = 200} work?
There was a problem hiding this comment.
Yes. I'll change it 👍
| before_each(function() | ||
| stub(test_counter, 'inc') | ||
| local Prometheus = require('apicast.prometheus') | ||
| getmetatable(Prometheus).__call = function(_, type) |
There was a problem hiding this comment.
Doing this dance in tests is so ugly and error-prone, that I think this could be an excellent time to fix this interface.
There was a problem hiding this comment.
Agreed. I thought the same because it's the second time we need to write something like this.
I was thinking about whether to modify it in this PR or the next one.
There was a problem hiding this comment.
Let's address this one separately because it will change many things not directly related to this PR.
| end | ||
| end | ||
|
|
||
| package.loaded['apicast.metrics.3scale_backend_calls'] = nil |
There was a problem hiding this comment.
Similar comment as above. If we have to do this, looks like something is broken in our interface.
davidor
force-pushed
the
metrics-backend-calls-type
branch
from
ab04b0f
to
b200cea
Compare
davidor
force-pushed
the
metrics-backend-calls-type
branch
from
b200cea
to
c2fa0d0
Compare
Part of #745