Skip to content

Conversation

@zirain
Copy link
Member

@zirain zirain commented Nov 11, 2025

xref: envoyproxy/envoy#41697

this's useful when users want to add value REQUESTED_SERVER_NAME to tag.

@zirain zirain requested a review from a team as a code owner November 11, 2025 03:25
@codecov
Copy link

codecov bot commented Nov 11, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.33%. Comparing base (fb389ff) to head (09213f2).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7485      +/-   ##
==========================================
- Coverage   72.37%   72.33%   -0.04%     
==========================================
  Files         232      232              
  Lines       34143    34143              
==========================================
- Hits        24711    24698      -13     
- Misses       7659     7670      +11     
- Partials     1773     1775       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

rudrakhp
rudrakhp previously approved these changes Nov 13, 2025
@zirain zirain force-pushed the formatter-tag branch 2 times, most recently from 8620c96 to b37d450 Compare November 17, 2025 01:41
@arkodg arkodg added this to the v1.7.0-rc.1 Release milestone Nov 20, 2025
// Formatter adds value using formatter to each span.
// It's required when the type is "Formatter".
// +optional
Formatter *FormatterCustomTag `json:"formatter,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason we have 2 level here ?
does the similar field for access log use the same field name of formatter ?

Copy link
Member Author

Choose a reason for hiding this comment

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

there're two reasons why I used FormatterCustomTag here:

  1. keep same pattern with others type.
  2. in case there's more options in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Name does align with the changes from envoyproxy/envoy#41697

Signed-off-by: zirain <[email protected]>
Signed-off-by: zirain <[email protected]>
@arkodg
Copy link
Contributor

arkodg commented Dec 1, 2025

rethinking this one, do we need an explicit API field for now, in header.value case, we reuse the same field for literal and formatter string, can we do the same here ?

@zirain
Copy link
Member Author

zirain commented Dec 1, 2025

rethinking this one, do we need an explicit API field for now, in header.value case, we reuse the same field for literal and formatter string, can we do the same here ?

if there's no type, it's fine.

rethinking this one, do we need an explicit API field for now, in header.value case, we reuse the same field for literal and formatter string, can we do the same here ?

We can change the underlying layer of literal to this new formatter type, which is supported.

@arkodg
Copy link
Contributor

arkodg commented Dec 1, 2025

rethinking this one, do we need an explicit API field for now, in header.value case, we reuse the same field for literal and formatter string, can we do the same here ?

if there's no type, it's fine.

rethinking this one, do we need an explicit API field for now, in header.value case, we reuse the same field for literal and formatter string, can we do the same here ?

We can change the underlying layer of literal to this new formatter type, which is supported.

sure sgtm

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.

4 participants