Skip to content

Conversation

@agrawroh
Copy link
Member

Description

This PR adds optional stats collection to Header-To-Metadata filter when enabled. It's quite helpful to understand when the header was missing, how many rules get processed, failures etc.


Commit Message: h2m: add optional stats to header-to-metadata filter
Additional Description: Adds optional stats collection to Header-To-Metadata filter when enabled.
Risk Level:
Low
Testing: Added Tests
Docs Changes: Added
Release Notes: Added

@repokitteh-read-only
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #40464 was opened by agrawroh.

see: more, trace.

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

🐱

Caused by: #40464 was opened by agrawroh.

see: more, trace.

@agrawroh agrawroh marked this pull request as ready for review July 27, 2025 22:33
@agrawroh agrawroh requested a review from zuercher as a code owner July 27, 2025 22:33
@phlax
Copy link
Member

phlax commented Aug 4, 2025

@agrawroh could you rebase please

ping @wbpcode

@phlax
Copy link
Member

phlax commented Aug 7, 2025

may need to wait

Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. And I have added some comments first.

/wait

Comment on lines 229 to 231
if (config->stats().has_value()) {
config->stats().value().base64_decode_failed_.inc();
}
Copy link
Member

Choose a reason for hiding this comment

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

I will inclined to createa method in the Config to accept the two enums, one enum for event, one enum for traffic direction.
Then handle all this logic in same method to avoid this if (config->stats().has_value()) everywhere. If we need to add new stats in the future, it would much simpler also.

@wbpcode
Copy link
Member

wbpcode commented Aug 9, 2025

/lgtm api

@repokitteh-read-only repokitteh-read-only bot removed the api label Aug 9, 2025
Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

Thanks for update and LGTM overall. Only one last comment.

Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@agrawroh agrawroh merged commit 49c5c95 into envoyproxy:main Aug 14, 2025
25 checks passed
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.

3 participants