Skip to content

envoy file provider support omit_empty_values#3426

Merged
istio-testing merged 2 commits into
istio:masterfrom
zirain:accesslog/omit_empty_values
Feb 27, 2025
Merged

envoy file provider support omit_empty_values#3426
istio-testing merged 2 commits into
istio:masterfrom
zirain:accesslog/omit_empty_values

Conversation

@zirain

@zirain zirain commented Jan 31, 2025

Copy link
Copy Markdown
Member

@zirain
zirain requested a review from a team as a code owner January 31, 2025 14:04
@istio-testing istio-testing added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 31, 2025
@zirain zirain added the release-notes-none Indicates a PR that does not require release notes. label Feb 2, 2025
@zirain

zirain commented Feb 2, 2025

Copy link
Copy Markdown
Member Author

/test release-notes

@zirain zirain changed the title envoy file provider support envoy file provider support omit_empty_values Feb 2, 2025
// Optional. If set to true, when command operators are evaluated to null,
// For text format, the output of the empty operator is changed from “-“ to an empty string.
// For json format, the keys with null values are omitted in the output structure.
bool omit_empty_values = 3;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

by default is it is empty, isn't it difficult to distinguish which field is empty?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

for text, yes.
but it's useful for json.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So can we only make it work for json

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think it's fine to give user the option, instead of make an exception.
User can use customized separator, e.g. [%CEL(xxxxxx)%].

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 14, 2025
@zirain
zirain force-pushed the accesslog/omit_empty_values branch from a307350 to 600974e Compare February 14, 2025 12:22
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 14, 2025
@zirain

zirain commented Feb 22, 2025

Copy link
Copy Markdown
Member Author

kindly ping @istio/technical-oversight-committee

@keithmattix keithmattix left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A couple of things:
1: I'm not totally sure what the use case is even after reading the linked issue.
2. Why MeshConfig over Telemetry API?

@zirain

zirain commented Feb 26, 2025

Copy link
Copy Markdown
Member Author

I'm not totally sure what the use case is even after reading the linked issue.

Before, envoy will send "key": "" in json
After, envoy will remove the field if the value is empty.

Why MeshConfig over Telemetry API?

Provider (in MeshConfig) is part of Telemetry API.
In the past, we talked about make it a separated CRD or else, but didn't make a aggrement.

@zirain
zirain requested a review from keithmattix February 27, 2025 03:25
@istio-testing
istio-testing merged commit b047b1a into istio:master Feb 27, 2025
@zirain
zirain deleted the accesslog/omit_empty_values branch February 27, 2025 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes-none Indicates a PR that does not require release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants