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

feat(log-serializer): add source property to log-serializer #12052

Merged
merged 5 commits into from
Nov 29, 2023

Conversation

vm-001
Copy link
Contributor

@vm-001 vm-001 commented Nov 17, 2023

Summary:

Add a new property source to indicate the response is generated by kong or upstream.

FTI-5522

Copy link
Member

@oowl oowl left a comment

Choose a reason for hiding this comment

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

Most LGTM, But I want to clarify that not all the response code errors can be captured by Kong Lua code, Like 400 or Nginx internal error. it can not be captured in our implementation of get_source PDK.

@vm-001
Copy link
Contributor Author

vm-001 commented Nov 17, 2023

@oowl Are you referring to the internal EE PR? Because this PR only adds one propertysource, the response.upstream_status was done by another PR a long time ago. See #10296

The source value comes from kong.response.get_source, which has a default value error, and will be transformed to kong.

@locao locao requested review from zhongweiy and gszr November 21, 2023 17:44
@zhongweiy zhongweiy removed their request for review November 21, 2023 18:25
Copy link
Member

@gszr gszr left a comment

Choose a reason for hiding this comment

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

very minor comments; otherwise, LGTM

kong/pdk/log.lua Outdated Show resolved Hide resolved
@vm-001 vm-001 requested a review from gszr November 24, 2023 06:23
kong/pdk/log.lua Outdated
Comment on lines 821 to 825
if response_source == "exit" or response_source == "error" then
response_source = "kong"
elseif response_source == "service" then
response_source = "upstream"
end
Copy link
Member

Choose a reason for hiding this comment

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

so okong.response.get_source(ongx.ctx) only have 3 values , exit, error or service?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct.

kong/kong/pdk/response.lua

Lines 345 to 364 in 6077171

function _RESPONSE.get_source(ctx)
if ctx == nil then
check_phase(header_body_log)
ctx = ngx.ctx
end
if ctx.KONG_UNEXPECTED then
return "error"
end
if ctx.KONG_EXITED then
return "exit"
end
if ctx.KONG_PROXIED then
return "service"
end
return "error"
end

Copy link
Member

@tzssangglass tzssangglass Nov 24, 2023

Choose a reason for hiding this comment

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

I prefer to use a constant table to contain these three values to prevent if someone change function _RESPONSE.get_source(ctx) but forget to update it here.

Copy link
Member

Choose a reason for hiding this comment

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

@vm-001 any thoughts?

Copy link
Contributor Author

@vm-001 vm-001 Nov 29, 2023

Choose a reason for hiding this comment

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

I don't have any strong opinions on this. Just feel replacing the if conditions with a constant table may be not completed. The vars defined in _RESPONSE.get_source function should also be changed. Please ref the latest commit to get what I'm saying.

Copy link
Member

Choose a reason for hiding this comment

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

I understand @vm-001's point and agree this would be an improvement; in the interest of keeping things moving, let's merge this as-is, and, if you both think this is a desirable improvement, please create a JIRA to track it.

Copy link
Contributor Author

@vm-001 vm-001 Nov 30, 2023

Choose a reason for hiding this comment

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

Thank you @gszr. @tzssangglass Checkout the latest commit to see if it addresses your concern. If yes. please resolve this comment. Otherwise, open a ticket then we can track it.

@vm-001 vm-001 force-pushed the feat/upstream-status-log-serializer branch from 53e67d9 to d2604f8 Compare November 29, 2023 02:58
@vm-001 vm-001 force-pushed the feat/upstream-status-log-serializer branch from d2604f8 to 436ff7d Compare November 29, 2023 03:24
@gszr gszr merged commit 2784bf5 into master Nov 29, 2023
24 checks passed
@gszr gszr deleted the feat/upstream-status-log-serializer branch November 29, 2023 17:27
@vm-001 vm-001 added cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee and removed cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Nov 30, 2023
@outsinre outsinre added the cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee label Jan 25, 2024
@team-gateway-bot
Copy link
Collaborator

Successfully created cherry-pick PR for master:

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 core/logs core/pdk size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants