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

generic proxy: non template formatter/logger support #38164

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wbpcode
Copy link
Member

@wbpcode wbpcode commented Jan 23, 2025

Commit Message: generic proxy: non template formatter/logger support

Additional Description:

In the past, to support multiple protocols, we introduced the templated formatter and loggers.We can create different FormatterContext class for different protocols. And for different FormatterContext, we can register different substitution commands.

It works fine, but its code is pretty complex to hard to read/maintain.

There are too much code need to be templated. Formatter, FormatterProvider, CommandParser, CommandParserFactory, AccessLog::Instance, AccessLog::Filter, AccessLog::InstanceFactory, AccessLog::ExtensionFilterFactory, etc.....

This PR refactor the generic proxy's logger and formatter to re-use the HttpFormatterContext directly and use a virtual Extension to store non-HTTP specific context.

After this PR, we can change all above template classes back to normal classes.

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

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 @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #38164 was opened by wbpcode.

see: more, trace.

@wbpcode wbpcode removed the api label Jan 23, 2025
@wbpcode wbpcode assigned adisuissa and unassigned markdroth Jan 23, 2025
@wbpcode wbpcode added the api label Jan 23, 2025
@wbpcode
Copy link
Member Author

wbpcode commented Jan 23, 2025

Another good side effect of this PR is now, the telmetry logger, fluend logger, stream logger, etc. could be used in the generic proxy directly without any code change. Becasue now the generic proxy share completely same loggers with HTTP. (Except the generic proxy has some specific substitution commands support.)

@wbpcode
Copy link
Member Author

wbpcode commented Jan 23, 2025

cc @alyssawilk I think you have asked some related quesions about these code.

@wbpcode
Copy link
Member Author

wbpcode commented Jan 23, 2025

/retest

//
// If the ``envoy.access_loggers.file`` logger is used for generic proxy, this formatter will
// be enabled by default.
message GenericProxyLogFormatter {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I don't grok what's the purpose of this. Maybe it is because I'm not familiar with generic-proxy.
Is there some place that describes why the normal formatter types cannot be used for the loggers of a generic-proxy?

Also, from the description above, it is unclear to me where this type can/should be used.

Copy link
Member Author

@wbpcode wbpcode Jan 24, 2025

Choose a reason for hiding this comment

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

This is used for log formatter. Envoy provides some built-in substitution commands for logging, like %REQ(:PATH)%, %UPSTREM_HOST%, etc. When the format string is loading, all the commands will be parsed as providers and be evaluated when doing logging. If there are some unsupported commands like %UNKNOWN%, then the coonfiguration will be rejected.

But Envoy also provides an extension pointer to extend custom substitution commands. See formatters field of https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/core/v3/substitution_format_string.proto. (This exention pointer could be used by file/stream loggers. And fluentd logger, opentelmetey logger have provided another same extension pointer).

We add this new empty proto message for generic-proxy-specific substitution commands support. Like %GENERIC_STATUS_CODE%, etc.

Here is a configuration example with fluentd logger:

access_log:
- name: fluentd_logger
  typed_config:
    "@type": type.googleapis.com/envoy.extensions.access_loggers.fluentd.v3.FluentdAccessLogConfig
    cluster: xxxxxx
    record:
      generic_proxy_status_code: %GENERIC_STATUS_CODE%
      generic_proxy_request_property: %REQUEST_PROPERTY(test_property_name)%
      common_dration_from_stream_info: %DURATION%
      upstream_host: %UPSTREM_HOST%
    formatters:
    - name: generic_proxy_specific_formatter_commands_support
      typed_config:
        "@type": "@type": type.googleapis.com/envoy.extensions.filters.network.generic_proxy.v3.GenericProxySubstitutionFormatter

I will set this extension point for file logger by hard code for backward compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants