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.

Sign up for GitHub

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

Jump to bottom

udp_proxy: support proxy access logging #23242

Merged
merged 16 commits into from
Nov 1, 2022
Merged

udp_proxy: support proxy access logging #23242

merged 16 commits into from
Nov 1, 2022

Conversation

giantcroc
Copy link

Signed-off-by: giantcroc changran.wang@intel.com

Commit Message:
Some stats like no_route and idle_timeout can't be printed by session access log,
so we need proxy-level access logging to log global stats.
Additional Description:
Risk Level: Low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue] #23241
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

giantcroc added 4 commits September 26, 2022 10:48
add proxy_access_log
45eaba4 
Signed-off-by: giantcroc <changran.wang@intel.com>
complete proxy access logging
4c7e21f 
Signed-off-by: giantcroc <changran.wang@intel.com>
add tests
ff5c635 
Signed-off-by: giantcroc <changran.wang@intel.com>
add docs
b4e9a96 
Signed-off-by: giantcroc <changran.wang@intel.com>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #23242 was opened by giantcroc.

see: more, trace.

@giantcroc
Copy link
Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #23242 (comment) was created by @giantcroc.

see: more, trace.

lizan
lizan reviewed Sep 26, 2022
View reviewed changes
Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

API review

api/envoy/extensions/filters/udp/udp_proxy/v3/udp_proxy.proto Outdated Show resolved Hide resolved
giantcroc added 2 commits September 27, 2022 14:01
fix name
e8f9f5a 
Signed-off-by: giantcroc <changran.wang@intel.com>
fix more names
241bdb6 
Signed-off-by: giantcroc <changran.wang@intel.com>
@giantcroc
Copy link
Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #23242 (comment) was created by @giantcroc.

see: more, trace.

@giantcroc
Copy link
Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #23242 (comment) was created by @giantcroc.

see: more, trace.

@giantcroc
Copy link
Author

@phlax Hi, it seems wierd that the old code is still used to do test

test/extensions/filters/udp/udp_proxy/udp_proxy_filter_test.cc:639:42: error: too few arguments to function call, expected 3, have 2
                        access_log_format));

Can you help with it? Thanks!

@lizan
Copy link
Member

lizan commented Sep 28, 2022

@giantcroc CI runs on merge commit and you can see that part code here: https://github.com/envoyproxy/envoy/blob/e143e084f5/test/extensions/filters/udp/udp_proxy/udp_proxy_filter_test.cc#L629-L639

It's a merge race with #23121, you just need to merge main branch and fix that test as well.

giantcroc added 2 commits September 29, 2022 16:28
@giantcroc
Copy link
Author

@giantcroc CI runs on merge commit and you can see that part code here: https://github.com/envoyproxy/envoy/blob/e143e084f5/test/extensions/filters/udp/udp_proxy/udp_proxy_filter_test.cc#L629-L639

It's a merge race with #23121, you just need to merge main branch and fix that test as well.

OK, fixed, thanks a lot!

Merge remote-tracking branch 'upstream/main' into proxy-level-logging
6d2805d 
Signed-off-by: giantcroc <changran.wang@intel.com>
@giantcroc
Copy link
Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #23242 (comment) was created by @giantcroc.

see: more, trace.

@phlax
Copy link
Member

phlax commented Sep 30, 2022

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #23242 (comment) was created by @phlax.

see: more, trace.

@lizan
Copy link
Member

lizan commented Oct 4, 2022

The coverage failure is a real one so you either need increase coverage or change threshold.

@lizan lizan added the waiting label Oct 5, 2022
Merge remote-tracking branch 'upstream/main' into proxy-level-logging
4d4e90f 
Signed-off-by: giantcroc <changran.wang@intel.com>
@giantcroc
Copy link
Author

giantcroc commented Oct 10, 2022 •
edited
Loading

The coverage failure is a real one so you either need increase coverage or change threshold.

Hi, lizan! Sorry for late reply. The coverage has been fixed after rebase. Thanks!

lizan
lizan previously approved these changes Oct 14, 2022
View reviewed changes
recover number
39fca93 
Signed-off-by: giantcroc <changran.wang@intel.com>
@lizan
Copy link
Member

lizan commented Oct 17, 2022

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #23242 (comment) was created by @lizan.

see: more, trace.

@lizan lizan added the waiting label Oct 17, 2022
Merge remote-tracking branch 'upstream/main' into proxy-level-logging
5903993 
Signed-off-by: giantcroc <changran.wang@intel.com>
recover field name
7251589 
Signed-off-by: giantcroc <changran.wang@intel.com>
ZiyangXiao
ZiyangXiao reviewed Oct 27, 2022
View reviewed changes
Copy link
Contributor

@ZiyangXiao ZiyangXiao left a comment

Choose a reason for hiding this comment

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

Should remove this line in this pr?

envoy/source/extensions/filters/udp/udp_proxy/udp_proxy_filter.h

Line 28 in 8266790

// TODO(mattklein123): UDP session access logging.

remove TODO
8401a41 
Signed-off-by: giantcroc <changran.wang@intel.com>
@giantcroc
Copy link
Author

Should remove this line in this pr?

envoy/source/extensions/filters/udp/udp_proxy/udp_proxy_filter.h

Line 28 in 8266790

// TODO(mattklein123): UDP session access logging.

OK, thanks!

@giantcroc
Copy link
Author

@lizan Hi, I have recovered the field name to address compatibility issue. Please help review, thanks!

lizan
lizan reviewed Oct 28, 2022
View reviewed changes
changelogs/current.yaml Outdated
@@ -39,6 +39,56 @@ new_features:

- area: http
change: |
added the expected :ref:`receive <envoy_v3_api_field_config.core.v3.HealthCheck.HttpHealthCheck.receive>` payload check for HTTP health check.
Copy link
Member

Choose a reason for hiding this comment

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

there's a lot of unrelated changes in this version history which might comes from resolving conflicts, please merge main again and add only what you did in this PR.

Copy link
Author

Choose a reason for hiding this comment

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

OK, fixed, thanks!

giantcroc added 2 commits October 31, 2022 10:30
Merge remote-tracking branch 'upstream/main' into proxy-level-logging
6c4b999 
Signed-off-by: giantcroc <changran.wang@intel.com>
fix changelogs
549042d 
Signed-off-by: giantcroc <changran.wang@intel.com>
@repokitteh-read-only repokitteh-read-only bot removed the api label Nov 1, 2022
@lizan lizan merged commit 33fce6b into envoyproxy:main Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daixiang0 daixiang0 daixiang0 left review comments

@ZiyangXiao ZiyangXiao ZiyangXiao left review comments

@lizan lizan lizan approved these changes

@mattklein123 mattklein123 Awaiting requested review from mattklein123 mattklein123 is a code owner

Assignees

@lizan lizan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@giantcroc @lizan @phlax @daixiang0 @ZiyangXiao