feat(accesslog): support config omit_empty_values

[j2gg0s]

Support user config omit_empty_values for Envoy's AccessLog, reference Issue/31060.

Related envoy's SubstitutionFormatString.omit_empty_values

[istio-policy-bot]

😊 Welcome @j2gg0s! This is either your first contribution to the Istio api repo, or it's been
awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

[istio-testing]

Hi @j2gg0s. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[zirain]

/ok-to-test

[j2gg0s]

@j2gg0s: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
release-notes_api 71e0d4d link false /test release-notes_api

this pr need release notes?

[zirain]

/test release-notes_api

[j2gg0s]

Any other thing than i need to fix?

[linsun]

Yes, can we add a release note for this pls?

[linsun]

Could we explain why this is useful and give 1 or two examples? I had to lookup the original issue and envoy docs.

[j2gg0s]

• omit_empty_values is atrribute of envoy's log format, istio should allow this feature to be use by anyone who need.
• As an example, we need log CONNECTION_TERMINATION_DETAILS only when connection be terminated，storing this field with null is waste.
• log with null is ugly

[douglas-reid]

@j2gg0s maybe the action here is to link to the envoy docs directly as a "for more information" style helper?

[j2gg0s]

@douglas-reid Did i understand correctly? With latest note

[douglas-reid]

That seems reasonable to me. @linsun wdyt ?

[istio-testing]

@j2gg0s: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
release-notes_api	3778328	link	false	/test release-notes_api

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[zirain]

kindly ping @linsun

[istio-testing]

@j2gg0s: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[akashjain971]

Anyone taking this forward?

[j2gg0s]

Anyone taking this forward?

@akashjain971 I switched to EnvoyFilters to manage logs, such as:

apiVersion: networking.istio.io/v1alpha3
kind: EnvoyFilter
metadata:
  name: log
  namespace: istio-system
spec:
  configPatches:
  - applyTo: NETWORK_FILTER
    match:
      listener:
        filterChain:
          filter:
            name: envoy.filters.network.http_connection_manager
    patch:
      operation: MERGE
      value:
        typed_config:
          '@type': type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
          accessLog:
          - name: envoy.access_loggers.file
            typedConfig:
              '@type': type.googleapis.com/envoy.extensions.access_loggers.file.v3.FileAccessLog
              logFormat:
                jsonFormat:
                  authority: '%REQ(:AUTHORITY)%'
                  ...
                  upstream_transport_failure_reason: '%UPSTREAM_TRANSPORT_FAILURE_REASON%'
                omit_empty_values: true
              path: /dev/stdout

[akashjain971]

Anyone taking this forward?

@akashjain971 I switched to EnvoyFilters to manage logs, such as:

apiVersion: networking.istio.io/v1alpha3
kind: EnvoyFilter
metadata:
  name: log
  namespace: istio-system
spec:
  configPatches:
  - applyTo: NETWORK_FILTER
    match:
      listener:
        filterChain:
          filter:
            name: envoy.filters.network.http_connection_manager
    patch:
      operation: MERGE
      value:
        typed_config:
          '@type': type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
          accessLog:
          - name: envoy.access_loggers.file
            typedConfig:
              '@type': type.googleapis.com/envoy.extensions.access_loggers.file.v3.FileAccessLog
              logFormat:
                jsonFormat:
                  authority: '%REQ(:AUTHORITY)%'
                  ...
                  upstream_transport_failure_reason: '%UPSTREAM_TRANSPORT_FAILURE_REASON%'
                omit_empty_values: true
              path: /dev/stdout

Thank you @j2gg0s. This should sail my boat for now but will this not add latency for every request?

[akashjain971]

Looks like OmitEmptyValues is already supported here.
This required Istio code changes and cannot be done from the IstioOperator.

[istio-policy-bot]

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2022-07-23. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.