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(aws-lambda): remove AWS Lambda API latency from kong latency #12835

Merged
merged 5 commits into from
Apr 25, 2024

Conversation

windmgc
Copy link
Member

@windmgc windmgc commented Apr 6, 2024

Summary

This commit removes the latency attributed to AWS Lambda API requests from Kong's measured latencies (latencies.kong, KONG_RESPONSE_LATENCY, X-Kong-Response-Latency), ensuring that these delays are not mistakenly considered as originating from Kong.

This is an alternative of #12678, but this PR fix the latency problem only within the AWS-Lambda plugin scope without introducing anything new, so that we can have a simple and appropriate fix for the user.

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

FTI-5261

@github-actions github-actions bot added plugins/aws-lambda cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Apr 6, 2024
This commit removes the latency attributed to AWS Lambda API
requests from Kong's measured latencies (latencies.kong,
KONG_RESPONSE_LATENCY, X-Kong-Response-Latency), ensuring
that these delays are not mistakenly considered as originating from Kong.
@windmgc windmgc force-pushed the aws-lambda-remove-lambda-latency-from-kong branch from 9933dc8 to 1025de2 Compare April 6, 2024 06:39
@liverpool8056
Copy link
Contributor

One more question:
Why is ctx.KONG_PROXIED not set in this plugin?
Two side effects for this (I'm not sure if they matter):

  • KONG_latencies.kong always respect KONG_RESPONSE_LATENCY instead of KONG_PROXY_LATENCY
  • The requests hitting the plugin are considered un-proxied.

@windmgc
Copy link
Member Author

windmgc commented Apr 8, 2024

@liverpool8056

Why is ctx.KONG_PROXIED not set in this plugin?

The KONG_PROXIED represents that the request will go to the upstream. In a serverless plugin like aws-lambda, plugin will call PDK's response function and use a delayed_response conditional block to do special treatment - because the requests will not go to the upstreams. And the access phase will end as soon as the ngx.exit() is being called inside the PDK function, so ctx.KONG_PROXIED will not be set

KONG_latencies.kong always respect KONG_RESPONSE_LATENCY instead of KONG_PROXY_LATENCY
The requests hitting the plugin are considered un-proxied.

Yes, this is what serverless plugin expects.

@windmgc windmgc added this to the 3.7.0 milestone Apr 15, 2024
@bungle
Copy link
Member

bungle commented Apr 23, 2024

Just a note here. We have huge number of plugins that do round trips and network io in synchronous fashion. AWS lambda is just one. We have Azure Functions, we have OIDC, we have OPA, OAuth Introspection, Session. A lot of our plugins communicate to Redis, which are beyond Kong, but somewhat on our land as well. Whenever there is network io, it is really hard to determine who's fault the added latencies are. It is hard to tell where our code stops and where other reasons start to affect. If we were to measure our non-yielding code only, then it is usually just around 0-1ms (the code execution time). But if we need to do network io on request time it gets wildly compilicated to count any numbers.

@windmgc
Copy link
Member Author

windmgc commented Apr 24, 2024

@bungle Yes, that is also what I'm thinking about. This PR's scope is to only solve the aws-lambda plugin without introducing anything new to have a quick fix for FTI-5261.

bungle
bungle previously requested changes Apr 24, 2024
Copy link
Member

@bungle bungle left a comment

Choose a reason for hiding this comment

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

@windmgc, Not a big deal, just thinking if it would be better to use kong.ctx.plugin here (feel free to disagree and mark this resolved).

kong/plugins/aws-lambda/handler.lua Outdated Show resolved Hide resolved
kong/plugins/aws-lambda/handler.lua Outdated Show resolved Hide resolved
Co-authored-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
@windmgc windmgc requested a review from bungle April 25, 2024 03:49
@jschmid1 jschmid1 dismissed bungle’s stale review April 25, 2024 12:39

comments were addressed

@jschmid1 jschmid1 merged commit bf673ce into master Apr 25, 2024
25 checks passed
@jschmid1 jschmid1 deleted the aws-lambda-remove-lambda-latency-from-kong branch April 25, 2024 12:40
@team-gateway-bot
Copy link
Collaborator

Successfully created cherry-pick PR for master:

tysoekong pushed a commit that referenced this pull request Apr 26, 2024
)

* fix(aws-lambda): remove AWS Lambda API latency from kong latency

This commit removes the latency attributed to AWS Lambda API
requests from Kong's measured latencies (latencies.kong,
KONG_RESPONSE_LATENCY, X-Kong-Response-Latency), ensuring
that these delays are not mistakenly considered as originating from Kong.

---------

Co-authored-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
(cherry picked from commit bf673ce)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee plugins/aws-lambda size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants