Add `StaticFilter` trait #515

XAMPPRocky · 2022-04-13T21:03:19Z

This PR is borne out of initially an effort to pull the yaml_to_protobuf changes to FilterFactory from #506 into a separate PR, and also add a protobuf_to_yaml method. However adding another new method was pretty tedious, and when thinking more holistically about the problem of converting configuration back and forth from YAML and Protobuf, I realised that we needed a more generic solution that would allow us to statically only define the types and have the same dynamic behaviour.

This thought process led me to this PR which adds a new StaticFilter trait which does exactly that. Now if you're writing a filter in Rust you no longer have to define a FilterFactory to construct it, you only implement StaticFilter and Filter, and then Quilkin will automatically at compile-time generate a virtual table safe version of StaticFilter (still called FilterFactory for the sake of not changing everything all at once).

This new trait design effectively eliminates all boilerplate needed when defining your own filter. For example; this is entire definition for the Compress as a filter (excluding the actual Filter impl). As shown, nearly everything is defined statically with types and constants, the only behaviour needed to be implemented is converting from an Option<Self::Configuration> into a Self.

impl StaticFilter for Compress {
    const NAME: &'static str = "quilkin.filters.compress.v1alpha1.Compress";
    type Configuration = Config;
    type BinaryConfiguration = proto::Compress;

    fn new(config: Option<Self::Configuration>) -> Result<Self, Error> {
        Ok(Compress::new(
            Self::ensure_config_exists(config)?,
            Metrics::new()?,
        ))
    }
}

It's hard to understate how much this single abstraction saves in terms of time and effort. The seperation provides two key things, the most obvious of which is mostly type-based API that is verified at compile-time for getting access to a filter and its configuration. The second benefit is that having a seperate trait allows us to split the dynamic dispatch FilterFactory functionality, this means that changes to FilterFactory have no affect on StaticFilter definitions. This is how this PR is able to add two safely add new FilterFactory methods which allow you to dynamically transcode the configuration format, and for it to still be net negative in terms of code added.

Breaking Changes

Filter modules no longer expose their name through a module level constant, instead preferring StaticFilter::NAME.
Filter modules no longer expose a factory free function, use StaticFilter::factory to achieve the same effect.

markmandel

This is a really nice refactor!!!

The "writing a customer filter" documentation needs a once over to update to this new architecture though (you may well have been waiting to do that post initial review).

markmandel · 2022-04-14T23:32:14Z

src/config/config_type.rs

-        Static: serde::Serialize
-            + for<'de> serde::Deserialize<'de>
-            + TryFrom<Dynamic, Error = ConvertProtoConfigError>,
+        BinaryConfiguration: prost::Message + Default,


That is a nice renaming 👍🏻

markmandel · 2022-04-14T23:33:10Z

src/config/config_type.rs

-            + for<'de> serde::Deserialize<'de>
-            + TryFrom<Dynamic, Error = ConvertProtoConfigError>,
+        BinaryConfiguration: prost::Message + Default,
+        TextConfiguration:


StringConfiguration ? (Rather than TextConfiguration)?

Just a possible suggestion, non-blocking.

I chose Text over String because String is a specific type in Rust as opposed to just "human readable", and it's shorter.

src/filters.rs

docs/src/filters/writing_custom_filters.md

bombsimon

Looks like a nice way to implement filters!

docs/src/filters/writing_custom_filters.md

examples/quilkin-filter-example/src/main.rs

github-actions · 2022-04-19T07:05:43Z