Maybe "ignored for all data callbacks" rather than "ignored going".

This is the first time the concept of "data callback" comes up. Is there a way to explain it more clearly?

What happens when the matcher is used in a non-HTTP filter context? E.g. in some new router scheme. I guess HTTP filter callbacks still apply there, but what about TAP, etc.?

In my mind this would only be used in the context of HTTP filters (hence the name SkipFilter). Right now we don't have a mechanism to allowlist which actions can be used in various contexts, but I imagine we'll want this or some kind of template trickery to create unique sets of actions for different contexts.

I've removed the use of "data callback", hopefully this will be easier to understand now

Nit: s/a/an/ here and below before ExtensionWithMatcher.

Nit: "makes heave use". I'd also maybe say "for performance better than linear list matching as seen in Envoy's HTTP routing" or the like to clearly provide the bogeyman that is at stake.

@phlax should this be a verified code example? Can we generally just enable this for all new code fragments?

the code that i have added verification for so far were complete configurations which are then added to the rst with literalinclude

we could just separate this into yaml and validate just that, or we could wrap this example in a complete config and just highlight the important parts

Do I get validation for free just by following the literalinclude pattern used elsewhere? Or do I need to do anything else?

It's easy enough to include enough config to make it a valid config, so if we can easily validate the config by doing that then I think it's worth it

its kinda the other way round in a sense

if you separate the config it will automatically get validated unless its specifically excluded

...at least for the envoy config - not sure about any yaml validation

Do I get validation for free just by following the literalinclude pattern used elsewhere? Or do I need to do anything else?

so, yep

Nit: prefer some RST formatting for filer proper names, e.g. *SetResponseCode*.

should this be a link to the relevant filter?

Surprises are fun :) Is there a way to call out some concrete examples of where this happens?

Sure I'll dig through and find an explicit example.

Down the line I would like to use #14727 to make it possible for filters to codify these to cause config rejection instead of these surprises, but this is what we've got for now

Nit: RST markup for function literals.

RST link to ExtensionMatcher def

It's a little confusing seeing both action name skip and the key here being skip_filter. Could the key (which is header value) be "some_value_to_match_on" or something like that?

A non-actionable comment that maybe we can just discuss thoughts on; the verbosity here is so high that I think we have two issues from a practical perspective:

1. Users who hand-write config are probably going to never want to use complex matchers. I feel the only way to use this reasonably is with some frontend language or API.
2. The config size is going to blow out at scale when we have type URL appearing everywhere. We've seen this also when looking at the impact of xdstp:// URLs. It would be ironic if a solution aimed at scalability has a high cost due to config size explosion. I think for this problem we can maybe just add in some fixed matchers over time that take the place of common operations. This can be future work, but I think at scale it might be needed (e.g. a route table with 100k entries).

For 1 I agree that people are probably never going to write complicated trees by hand: even for existing APIs most the advanced use cases are probably written at least using protobuf intellisense/autocomplete. Do you think this example ends up being too complicated to be useful?

I think this is a valid concern, though the config size explosion mostly matters at config update, not stream processing. At some point it can become an issue (CPU usage, delayed config application) but I don't think it would directly affect data plane performance. I wonder if it makes sense to reuse the pattern of static SDS secrets where we could define named extensions somewhere and then simply reference them by name in config instead of repeating the whole configuration. That would avoid the explosion in cases where the exact same matcher/input is used in a lot of different branches

Not arguing against including the example, just sharing some food for thought. I agree the cost is only at config update, that's where we're seeing real CPU costs at O(10k) cluster or route configs.

We've been thinking even about templating to make xdstp:// names work better, I'll keep this use case in mind as well when thinking through that.

Not sure about how to parse "By causing the filter to once the .."

i think this should have a new line

also new line here

Can you wire up the new [#extension-category] stuff here? See other recent examples. I'm surprised this wasn't already done. cc @phlax

I see this annotation is applied one layer higher. I would potentially move it here but up to you.

im not following i dont see the extension-category

There's // [#extension-category: envoy.filters.http] on L304 on the HttpFilter field declaration

This is really hard to read. Can you add more inline comments?

nit: I might consider moving this inside advanced? or at least lower in the list?

Is it worth putting an attention block in here with a link to the open issue tracking match schema checking or similar?

s/performance better/better performance/

s/beahviour/behaviour/