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

access log formatter: use new formatter context rather than multiple parameters (3/3) #30758

Merged

Conversation

wbpcode
Copy link
Member

@wbpcode wbpcode commented Nov 7, 2023

Commit Message: access log formatter: use new formatter context rather than multiple parameters (3/3)
Additional Description:

Continuous work of #30757. Waiting #30757 to be landed first.

The PR update the log Instance interface to be a specialization of InstanceBase with context type HttpFormatterContext.
And by this way, the log() method of Filter is updated to take a HttpFormatterContext reference as parameter.

This PR also do similar update to the AccessLogInstanceFactory and make it a specialization of AccessLogInstanceFactoryBase.

By this way, log instances of different modules will share the same code base.

Note, to ensure the backwards compatibility, the extension category of log instance should be kept as envoy.access_loggers.

This is the last PR of this big refactoring. After landed this PR, we will finally completely migrate our formatter/logger to use the context based interface.

This will bring two obvious benefits:

  1. it's easier to make the common part of these code be a template. All formatter-based loggers could be shared by different modules like dubbo proxy, generic proxy or even redis/mysql if necessary (some minor update is still necessary, but it's simple and minor).
  2. it's easier to extend the logger/formatter to add new feature like request trailer support. We needn't to change almost every log()/format() call to add a feature that only used in part of positions anymore. (think of the access log type 😥 ).

Risk Level: low.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

…parameters (1/3)

Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
…parameters (2/3)

Signed-off-by: wbpcode <wbphub@live.com>
…parameters (3/3)

Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
…ccess-log-formatter-part3

Signed-off-by: wbpcode <wbphub@live.com>
@wbpcode wbpcode marked this pull request as ready for review November 8, 2023 03:35
@wbpcode
Copy link
Member Author

wbpcode commented Nov 8, 2023

Related to #13085

Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

LGTM, but one suggestion.

source/common/http/filter_manager.h Outdated Show resolved Hide resolved
Signed-off-by: wbpcode <wbphub@live.com>
@wbpcode
Copy link
Member Author

wbpcode commented Nov 9, 2023

weird coverage result. the result tell wasm coverage rate is lower than the limit but from my local test result, it should be covered all.

Signed-off-by: wbpcode <wbphub@live.com>
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for doing this.

@RyanTheOptimist RyanTheOptimist merged commit 8dff697 into envoyproxy:main Nov 9, 2023
104 checks passed
@wbpcode wbpcode deleted the dev-access-log-formatter-part3 branch November 9, 2023 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants