Skip to content

accesslog: support stat-level AccessLogFilter in stats AccessLog#42676

Closed
TAOXUY wants to merge 4 commits intoenvoyproxy:mainfrom
TAOXUY:accessLogFilter
Closed

accesslog: support stat-level AccessLogFilter in stats AccessLog#42676
TAOXUY wants to merge 4 commits intoenvoyproxy:mainfrom
TAOXUY:accessLogFilter

Conversation

@TAOXUY
Copy link
Copy Markdown
Contributor

@TAOXUY TAOXUY commented Dec 17, 2025

Commit Message: with the PR, we can filter out stats based on their on AccessLogFilter within accesslogger. The use case can be like

  • The gauge support can rely on this PR to add/subtract by defining multiple gauges with different log_type_filter(TcpConnectionStart, TcpConnectionEnd, etc).
  • we can have counters for connection error by using response_flag_filter(one of AccessLogFilter) isn't empty

Risk Level: no risk
Testing: added
Docs Changes: no need
Release Notes: no need as the stats AccessLog extension hasn't been released.
Platform Specific Features: no

@repokitteh-read-only
Copy link
Copy Markdown

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

🐱

Caused by: #42676 was opened by TAOXUY.

see: more, trace.

@TAOXUY
Copy link
Copy Markdown
Contributor Author

TAOXUY commented Dec 17, 2025

@kyessenov @ggreenway

Copy link
Copy Markdown
Member

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

To be consistent with other access loggers, using

AccessLogFilter filter = 2;
to filter, and then having multiple instances of this access logger seems preferable. And that is already implemented.

Is there a use case where that wouldn't be sufficient?

/wait-any

@TAOXUY TAOXUY changed the title accesslog: support AccessLogFilter in stats AccessLog accesslog: support stat-level AccessLogFilter in stats AccessLog Dec 18, 2025
@TAOXUY
Copy link
Copy Markdown
Contributor Author

TAOXUY commented Dec 18, 2025

To be consistent with other access loggers, using

AccessLogFilter filter = 2;

to filter, and then having multiple instances of this access logger seems preferable. And that is already implemented.
Is there a use case where that wouldn't be sufficient?

/wait-any

Updated the description. PTAL

Signed-off-by: Xuyang Tao <taoxuy@google.com>
Signed-off-by: Xuyang Tao <taoxuy@google.com>
Signed-off-by: Xuyang Tao <taoxuy@google.com>
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @agrawroh

🐱

Caused by: #42676 was synchronize by TAOXUY.

see: more, trace.

@TAOXUY
Copy link
Copy Markdown
Contributor Author

TAOXUY commented Dec 18, 2025

Sry for making a push by mistake and loop in so many peoples. I am not maintainers and I cannot unassign the reviewers unfortunately.

@ggreenway
Copy link
Copy Markdown
Member

@wbpcode this feels weird to me from an API level because there's also a filter outside the access logger. But I can see how this would be useful in lowering the amount of config needed for some cases. WDYT?

@ggreenway ggreenway assigned ggreenway and kyessenov and unassigned agrawroh Dec 18, 2025
Comment thread api/envoy/extensions/access_loggers/stats/v3/stats.proto Outdated
@ravenblackx ravenblackx removed their request for review December 19, 2025 17:37
@kyessenov
Copy link
Copy Markdown
Contributor

I don't have a strong opinion on adding this API - will let API folks decide. I think you are not saving much on the costs yet (you could have two access logs with two filters, instead of one access log filter each with filter), unless I'm missing something. Generally, you want to evaluate predicate as early as possible, so it's calling for some nested matcher support down the line. Access log filters are difficult to use IMO, harder than it should be.

@TAOXUY
Copy link
Copy Markdown
Contributor Author

TAOXUY commented Dec 19, 2025

you could have two access logs with two filters, instead of one access log filter each with filter

That's a good point actually. Though feasible, it seems not ideal.

@ravenblackx
Copy link
Copy Markdown
Contributor

I believe this PR is waiting for API name change to be reversed.

/wait

Signed-off-by: Xuyang Tao <taoxuy@google.com>
@TAOXUY
Copy link
Copy Markdown
Contributor Author

TAOXUY commented Dec 29, 2025

I believe this PR is waiting for API name change to be reversed.

/wait

updated.

@ravenblackx
Copy link
Copy Markdown
Contributor

ravenblackx commented Dec 29, 2025

updated.

No commit has been pushed to the PR. (The waiting label will automatically be removed when a change is pushed.)

Edit: even if github then shows the push as occurring in the past so that the sequence of events looks weird!

@ggreenway
Copy link
Copy Markdown
Member

I still think this is an awkward API with two different levels of the same filtering. @wbpcode can you weigh in as the api-shepherd on this PR?

@kyessenov kyessenov removed their assignment Jan 8, 2026
@TAOXUY
Copy link
Copy Markdown
Contributor Author

TAOXUY commented Jan 14, 2026

I still think this is an awkward API with two different levels of the same filtering. @wbpcode can you weigh in as the api-shepherd on this PR?

Hi @ggreenway , not only

I still think this is an awkward API with two different levels of the same filtering. @wbpcode can you weigh in as the api-shepherd on this PR?

Another use case as I put above is that we can use response_flag_filter(one of AccessLogFilter) to filter out connections with errors.

@TAOXUY
Copy link
Copy Markdown
Contributor Author

TAOXUY commented Jan 14, 2026

Close for now, I will make another PR for filter tag value.

@TAOXUY TAOXUY closed this Jan 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api deps Approval required for changes to Envoy's external dependencies

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants