Skip to content
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

Runscope plugin #802

Closed
wants to merge 13 commits into from
Closed

Runscope plugin #802

wants to merge 13 commits into from

Conversation

mansilladev
Copy link
Contributor

No description provided.

timestamp = ngx.req_start_time,
response_time = ((ngx.ctx.kong_processing_access or 0) +
(ngx.ctx.kong_processing_header_filter or 0) +
(ngx.ctx.kong_processing_body_filter or 0) +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those ngx.ctx values slightly changed in next to be more explicit: https://github.com/Mashape/kong/blob/next/kong/plugins/log-serializers/basic.lua#L27


ok, err = sock:connect(host, port)
if not ok then
ngx.log(ngx.ERR, "[http-log] failed to connect to "..host..":"..tostring(port)..": ", err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/[http-log]/[runscope]/?

It is also best to cache the ngx.log and ngx.ERR variables since it accesses the Lua globals to retrieve ngx.

local ngx_log = ngx.log
local ngx_ERR = ngx.ERR

(This is done in the handler.lua file for example)

@thibaultcha
Copy link
Member

Very nice! I left a few comments about some details that need to be fixed.

I have two questions though:

  • Wouldn't it be best to just name it runscope? The mashape-analytics plugin will be named galileo, and the Loggly plugin is named loggly, same for syslog. Granted they have "log" in their name but the consistency here is that all plugins are simply named after the plugin they are integrating Kong with.
  • I remember you wanted to implement a queue mechanism and I suggested lua-resty-logger-socket. What decided you not to implement this mechanism? (Time consumption or is there any other reason that has to do with Runscope itself, would be a way to reformulate my question).

@mansilladev
Copy link
Contributor Author

@thibaultcha Re: queuing mechanism -- pushing that out. Needed more time to grok that library, plus wasn't sure how bleeding edge that was (the README wasn't very assuring). Also, renamed to runscope.

@mansilladev
Copy link
Contributor Author

@thibaultcha Can you look things over? I believe all optimizations you've recommended have been implemented.

response_time = ((ngx.ctx.KONG_ACCESS_TIME or 0) +
(ngx.ctx.KONG_RECEIVE_TIME or 0) +
(ngx.var.upstream_response_time * 1000) +
(ngx.var.request_time * 1000)) / 1000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this part is wrong.

  • ngx.ctx.KONG_ACCESS_TIME: time for Kong to run the access_by_lua directive, aka the time it took Kong before the request was sent to upstream, in ms.
  • ngx.ctx.KONG_RECEIVE_TIME: time spent waiting for the upstream response headers by Kong, in ms.
  • ngx.var.upstream_response_time: same as KONG_RECEIVE_TIME, except that it can contain a comma separated list of values if the request was retried by nginx_proxy (that can happen under load, see the proxy_next_* directives of nginx_proxy).
  • ngx.var.request_time:time elapsed between the first bytes were read from the client and the log write after the last bytes were sent to the client`

So this value will actually be 2 to 3 times the actual processing time:

ngx.ctx.KONG_ACCESS_TIME  |  |-----------|                         |
ngx.ctx.KONG_RECEIVE_TIME |              |-------------|           |
$upstream_response_time   |              |-------------|           |
$request_time             |----------------------------------------|

The ngx.ctx.* variable were added to avoid handling the case when var variables are comma-separated strings, causing Lua to failing the +/* etc operators.

Now, I am not sure what the response_time value is supposed to represent (time it took for the client (downstream) to get a response from upstream, through Kong?) If so, the most accurate way is to mimic what the ALF serializer does: alf.lua#L110 by adding the PROXY_LATENCY (time taken for Kong to receive and proxy to upstream) + WAITING_TIME (time elapsed until Kong receives the response headers) + RECEIVE_TIME (time taken for the response body to be proxied back to downstream). Those values are also already in ms resolution.

If this is what response_time is supposed to reflect, I can make those changes myself if you want.

@thibaultcha thibaultcha added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/needs review labels Dec 17, 2015
@mansilladev
Copy link
Contributor Author

@thibaultcha Ignore -- there's a bug. Timings are showing up as zeros. Digging in.

@thibaultcha
Copy link
Member

Okay, here if help needed

@mansilladev
Copy link
Contributor Author

Reapplied Runscope plugin code to 'next' branch. Closing out this PR. #863

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants