Add documentation entry for filter values

[iffyio]

We now have the concept of filter value https://github.com/googleforgames/quilkin/blob/master/src/extensions/filter_registry.rs#L36 and would like to mention this in the documentation - this can be done e.g in a sub-section of the filter documentation

[markmandel]

This is an excellent idea. In fact an overarching documentation on the concepts of Filters and how they are implemented would be a good idea.

[iffyio]

Context: #118 (comment)

With 'Filter invocation values' I'm thinking it isn't very descriptive on its own and more describes how the data is used rather than what it is - e.g hearing 'Filter Invocation Values' - my guess might be the contents of DownstreamContext/UpstreamContext since that's what we call a filter with.
Especially since most users won't know about the values field as that requires reading src code or docs on how to build your own filter - there won't be much of a connection to make.

Envoy seems to provide something similar to this: metadata and per-request/connection state to, from and between filters
where in our case rather than per-request its more per packet?

They have the concept of dynamic state and lower level call it dynamic metadata
I'm thinking this makes sense - if we can say that values contains (extra? dynamic?) information that a filter or other logic needs in order to process the packet (which sounds accurate to me), then we could call it (dynamic? if we want to be explicit that it differs from e.g src address that never change and are part of the struct) Packet Metadata as well with values => metadata? this would also capture the fact that it is per-packet info rather than global or session-based

[markmandel]

Excellent suggestion going to check on Envoy.

I definitely like this idea of "dynamic state"! - this follows nicely with Envoy, and I think the concepts overlap nicely.

I'd also be happy to rename values ➡️ state or maybe even dynamic_state (although that might be too much)?

I don't mind the idea of referring to this instead as "metadata" - but for some reason I feel like metadata is more descriptive of the packet... rather than being transitive state of the invocation... but ultimately, it probably doesn't actually matter.

I'm starting to wonder if UpstreamContext and DownstreamContext should be Upstream/DownstreamRequest ? Or something - The Response objects are still also part of the context.

Just thought I'd throw this out there as well, just as a thought to see if it stuck? DownstreamContextRequest and DownstreamContextResponse ? That seems a bit too verbose though. I dunno, just wanted to see if it made sense.

[iffyio]

I would prefer dynamic metadata to dynamic state thinking state would better suit more generic and longer lived data. Envoy's context is as a per-filter storage when processing all packets for a TCP or HTTP connection so state makes sense - if we later introduce storage that span a session, then I think state would be more appropriate but in our case the context is in processing a single packet (and the naming might collide if we do introduce session storage) and metadata feels lighter weight as such.

I feel like metadata is more descriptive of the packet... rather than being transitive state of the invocation

My thinking was that yeah to some extent it does contain info about the packet, in that, anything added to it is supposed to be in the context of processing the packet e.g the auth token is arguably metadata about the packet, and that they only really differ from the regular static metadata like address and port in that they're generated on the fly, hence the dynamic part .

I'm starting to wonder if UpstreamContext and DownstreamContext should be Upstream/DownstreamRequest ? Or something - The Response objects are still also part of the context.

Ah, In my mind the Response part of e.g DownstreamResponse was more response from the filter function rather than response to the packet while Context in DownstreamContext is conditions in which a downstream packet should be processed
I see them only as a grouping of arguments to the filter functions rather than a concept in themself - e.g I wouldn't mind having something like OnDownstreamReceiveArgs/OnDownstreamReceiveResult to clarify that that's all that they are.

I think Request and Response wouldn't fit very well because it'll suggest a request/response flow which won't be correct - the closest we have to a connection/request flow is sessions and by the time DownstreamContext is in play in the filterchain, the packet doesn't belong to any session which leads me to think that Request might too much of a generalization

[markmandel]

My thinking was that yeah to some extent it does contain info about the packet, in that, anything added to it is supposed to be in the context of processing the packet e.g the auth token is arguably metadata about the packet, and that they only really differ from the regular static metadata like address and port in that they're generated on the fly, hence the dynamic part .

That makes sense. I also don't have super strong feelings between the two. So sounds like we're calling it dynamic metadata then! 👍

I'm also assuming then that we'll also rename values ➡️ metadata ? That seems like it would make sense?

I think Request and Response wouldn't fit very well because it'll suggest a request/response flow which won't be correct - the closest we have to a connection/request flow is sessions and by the time DownstreamContext is in play in the filterchain, the packet doesn't belong to any session which leads me to think that Request might too much of a generalization

Makes sense too. Like I said - wanted to see if it stuck. Seems like no change required there then! 👍

[iffyio]

Sounds good! This ticket will cover the following then:

• Add Dynamic Metadata entry to the filter documentation
• Update values in the Downstream/UpstreamContext structs to metadata

[markmandel]

Also will want to switch valuesKey to metadataKey?

quilkin/src/extensions/filters/capture_bytes/mod.rs

Lines 49 to 51 in 12fc6ec

	#[serde(rename = "valuesKey")]
	#[serde(default = "default_values_key")]
	values_key: String,

[markmandel]

Confirming before closing - but I think this is all good to close as we've completed this here:
https://github.com/googleforgames/quilkin/blob/main/docs/extensions/filters/filters.md#filter-dynamic-metadata

[iffyio]

Yes! we can close this