Reapplied Runscope plugin to next branch #863
Conversation
|
@thibaultcha The ngx.ctx timing problems I encountered were due to being out of sync with |
|
Thanks I will check this out this soon! |
| @@ -121,6 +121,7 @@ build = { | |||
|
|
|||
| ["kong.plugins.log-serializers.basic"] = "kong/plugins/log-serializers/basic.lua", | |||
| ["kong.plugins.log-serializers.alf"] = "kong/plugins/log-serializers/alf.lua", | |||
| ["kong.plugins.log-serializers.alf"] = "kong/plugins/log-serializers/runscope.lua", | |||
There was a problem hiding this comment.
This module cannot have the same name as the already existing one above.
| end | ||
|
|
||
| if eof then -- latest chunk | ||
| ngx.ctx.runscope.response_received = ngx_now() * 1000 |
There was a problem hiding this comment.
It seems this is unused but could be the response.timestamp field of the Runscope message format?
| size_bytes = ngx.var.bytes_sent, | ||
| body = ngx.ctx.runscope.res_body, | ||
| timestamp = ngx.now(), | ||
| response_time = ngx.now() - ngx.req.start_time() |
There was a problem hiding this comment.
response_time
ngx.var.request_time will be the same value (seconds with a ms resolution) but already computed. It would just need to be converted to seconds.
One concern though: it will also include the time taken by Kong to send the response to the client. If we want to monitor the time it took for Kong to just receive the (full) response from upstream: (ngx.ctx.KONG_RECEIVE_TIME or 0) + (ngx.ctx.KONG_WAITING_TIME or 0) is probably what we want (they are in ms so if seconds if required, some conversion must also take place).
timestamp
ngx.now() will not return a UNIX timestamp but a floating point number in seconds with a milliseconds decimal part. Some conversion must take place here too.
response.size_bytes
It is said in the Runscope documentation:
The size of the response body in bytes.
Hence, the correct variable here is ngx.var.body_bytes_sent, since body_bytes also include the headers size. Also: this will not be the size of the response body received, but the one sent to the client. Meaning that if any other plugin modifies the response body, the size will be inaccurate. This is a limitation to be aware of, thought the size could also be computed manually from ngx.ctx.runscope.res_body.
request.url
The same applies here as to response.body: if a plugin modified the request's URI, the changes will not be reflected, this will be the request that Kong initially received, not the request made to the upstream API.
Overall, I think what matters more here is that the "client" of the API really is Kong, since you want to monitor how your API is doing by itself, and do as if Kong was just a regular client, right? To get metrics on how Kong itself performs for your API 9or Kong + API latencies for example), that would be a different tool than Runscope, is the way I see this for now. Hence why I am proposing those changes, so the plugin can most accurately achieve Runscope's goal.
There was a problem hiding this comment.
response_time
Keeping as is -- however, will replace with ngx.var.request_time * 1 (it's actually a string in seconds) since it's already calculated. Yes, we want to include the time it took Kong to send response to client. Network speed/latency needs to be taken into consideration because that is useful information for Runscope users.
timestamp
Keeping as is. Runscope accepts a floating point for timestamp, so this works perfectly.
response.size_bytes
Change will be made to ngx.var.body_bytes_sent. Good catch.
request.url
Keeping as is. This is what we want to record on the Runscope end. Transformations are not the concern of the calling client.
|
@thibaultcha Let me know if you have any other feedback. Otherwise, it would be swell to get his merged and let some real use from the community spark more ideas. |
|
@mansilladev we're in the middle of releasing 0.6 so we can merge this one post release. In the meantime can you pls open a PR to the getkong.org repo with logo, description, documentation for the plugin gallery. |
|
@sinzone Thanks, will do. |
|
@sinzone - PR on getkong.org w/logo/docs for plugin submitted. Anything else required to get these merged? |
|
Code wise is ok but I will apply manually with minor tweaks.
|
|
@thibaultcha Perfect. Thanks. |
thibaultcha
added
pr/ready (but hold merge)
thibaultcha
removed
the
pr/ready (but hold merge)
|
I created #924 where I rebased on top of our current next to avoid conflicts, applied the patch manually adding some minor modifications and fixing the linter. |
ngx.ctx timings working properly now (the catalyst to this manual rebase).