-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[example] adds example for collecting response metrics #404
[example] adds example for collecting response metrics #404
Conversation
@@ -0,0 +1,24 @@ | |||
location = /report_metric { | |||
internal; | |||
resolver 8.8.8.8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we would not have to define a resolver here, but unlike the other conf blocks, the default resolver doesn't appear to be picked up here.
set $document_count $arg_count; | ||
set $user_credentials transactions[0][user_key]=$user_key; | ||
set $metrics transactions[0][usage][$metric_name]=$document_count; | ||
set $reporting_url https://su1.3scale.net:443; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We tried using $backend_endpoint
here, but it's value was "backend_upstream"
which resulted in a peering lookup error failed to set current backend peer: missing peers while connecting to upstream
.
Hi @mikz or other maintainers - any feedback on this PR? |
I plan to take a look and suggest some improvements. Surely this week. Sorry for taking so long.
5. 9. 2017 v 16:00, Niall Burkley <notifications@github.com>:
… Hi @mikz or other maintainers - any feedback on this PR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
No worries, thanks @mikz! |
@nburkley I think this is very important use case and we could try to build it inside the gateway. The plan would be following:
But back to the example. I think this is good use case and we will have to improve the way of writing such customizations by improving the foundations. I can either merge the example as it is and improve it later when those new APIs are available or try to work on this PR as I create those APIs and merge it as complete example. With the backend_client all the nginx customization could go away and just leave the custom lua module. |
@mikz Thanks for the helpful feedback. I'm very happy to hear you're considering integrating logic like this directly into the gateway. Ideally, we'd prefer to customize our 3scale gateway a little as possible and handle all our business logic in our application layer or the 3scale admin, so enhancements like this would be very welcome.
We did look at using this initially, but we would like our API limits to be "soft limits" where API access is not disrupted when the limit is hit. The authrep call returns a From what I can see we would either need to modify the backend_clint to include a reporting call or have a custom nginx conf as we're currently doing.
I don't mind waiting to include this as a complete example. As I mentioned, I created this PR because we were having trouble finding the 'correct' way to do this ourselves and thought it might be helpful for someone else in a similar situation. If you think this example will make more sense once the other API changes have been made, then I'm happy to wait for a more complete example. In the meantime, if there are any other changes or improvements I can make to this, I'm happy to help. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nburkley just a couple of comments based on Michal's initial feedback. The more generic we can make this whilst still fitting your use case (which is a good one) the better.
-- only report metrics if the response was a success and the custom header exists | ||
if ngx.status == ngx.HTTP_OK and document_count then | ||
ngx.log(ngx.INFO, '[3scale-metrics] sending metric to 3scale document-count: ', document_count) | ||
local report = ngx.location.capture("/report_metric", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nburkley as @mikz pointed out it would be better to do this in lua. You could make 2 requests here to 3scale, first an authorise and then a report. This is done in the post_action phase and doesn't impact the latency on the client request. If the authorize returns a 409 you can allow the call through but if it returns a 403 then the call should be rejected, then you can report based on the authorize response. The authorize already exists in the backend_client so you would just need to add the report call there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can do that - I was trying to avoid modifying the backend_clint code - hence the call to an nginx conf.
I didn't have any auth-checks as I as going with the assumption that if the request made it thorugh our stack successfully it was already authorized - I can add this though.
end | ||
|
||
-- send custom metrics if this is a document search | ||
if ngx.var.request_uri:match '^/v1/documents' then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better if this regex is not hardcoded. One suggestion could be to define another environment variable that consists of the regex pattern(s) to match against. APICAST_REPORT_METRIC_MATCHER
for example... or at the very least define a variable with the regex pattern outside the scope of the function which can then be used anywhere within this custom module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, make sense, I'll tidy this up.
thanks for the feedback @kevprice83 - I'll ping you when I've updated the PR. |
We had a requirement to collect metrics on responses being returned to a user.
We couldn't find any existing solution to guide us so we build a custom module and config. We've created an example from our solution in the hopes that it helps someone else with a similar need.
I used examples/custom-config and examples/cors as templates when putting this together.
Any feedback is welcome!