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

fix(logging) include 'balancer_by_lua' latency in Kong latency #2494

Merged
merged 1 commit into from
Jun 23, 2017

Conversation

subnetmarco
Copy link
Member

  • fix(logging) include balancer_by_lua latency in Kong latency

Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

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

Sadly this won't do it. The balancer_by_lua directive can be invoked multiple times during a single request, and the current implementation in this patch will only consider the last invocation of the balancer handler.

You will need to distinguish the first invocation of the balancer handler (via the addr.try_count variable), and set the KONG_BALANCER_START variable only once. The same way, KONG_BALANCER_TIME can only be computed when we are sure that the current balancer invocation will be the last one.

It might be worth considering making this a list of entries, and have a different time for each try. After all, try 1 can, for various reasons (like DNS resolution, or load-balancing algorithm variations), be significantly slower of faster than another try. This would give useful debug information to a user if they were include in the serialized data sent by the logging plugins, for example.

@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/please review labels May 3, 2017
@subnetmarco subnetmarco requested review from Tieske and removed request for Tieske May 18, 2017 18:15
@p0pr0ck5
Copy link
Contributor

@thefosk do we still need this, or can we close this PR since we have a few debug/latency patches floating around?

@thibaultcha
Copy link
Member

We still need this, as this is missing from the current logging serializers, but as mentioned, a better version of it.

@subnetmarco
Copy link
Member Author

I will give this another look soon

@Tieske
Copy link
Member

Tieske commented May 29, 2017

you can use the tries field as it will contain info for each try (will be logged by serializers). See https://github.com/Mashape/kong/blob/master/kong/core/handler.lua#L108

As a side note: on its own the latency here will pretty much always be near 0ms, as the actual dns resolution is done in the access phase.

@thibaultcha
Copy link
Member

(cc @thefosk)

@Tieske Tieske force-pushed the fix/log-balancer-time branch 2 times, most recently from 5235fb0 to 25c73a2 Compare June 20, 2017 12:25
@Tieske
Copy link
Member

Tieske commented Jun 20, 2017

@thibaultcha @thefosk I updated this pr to properly track time spent per try. Please have a look.

@Tieske Tieske added pr/status/please review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Jun 20, 2017
@Tieske Tieske assigned Tieske and unassigned subnetmarco Jun 20, 2017
@thibaultcha thibaultcha modified the milestone: 0.11.0 Jun 23, 2017
@Tieske Tieske removed their request for review June 23, 2017 08:54
@thibaultcha thibaultcha force-pushed the fix/log-balancer-time branch from 25c73a2 to 3f3fe5b Compare June 23, 2017 17:35
@thibaultcha thibaultcha force-pushed the fix/log-balancer-time branch from 3f3fe5b to f5cfda0 Compare June 23, 2017 17:36
@thibaultcha
Copy link
Member

This one will do. Updated the commit author + moved the balancer callbacks to the right place in core.handler (after access). Thanks!

@thibaultcha thibaultcha added pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) and removed pr/status/please review labels Jun 23, 2017
@thibaultcha thibaultcha merged commit e27a7d6 into master Jun 23, 2017
@thibaultcha thibaultcha deleted the fix/log-balancer-time branch June 23, 2017 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants