Add proto config for all filters

[iffyio]

work on #10

[markmandel]

This should be proto3.

I'm not sure you can even use proto2 in grpc? 🤔 (apparently you can)

I'm guessing you did this because you can then use "optional" ?

[iffyio]

I started out with proto3 yeah it was in order to use optional. For the protoc compiler to generate optional types with proto3 it required --experimental_allow_proto3_optional to be passed in, but when I passed that into prost it panicked so I switched to proto2. If we would rather use proto3 we would probably need some workaround for optional things?

[markmandel]

So what I noticed from your previous example:
https://github.com/googleforgames/quilkin/blob/main/proto/quilkin/extensions/filters/debug/v1alpha1/debug.proto

If you use the wrapper type, it will generate a Option<String> for the Google imported managed types.

If you write your own wrapper type, (if I'm remembering correctly), you get something akin to Option<Wrapper>, which has the type you want in it - which is a bit more unwieldy, since you need to go option.unwrap().value to get to the underlying value you want, but it is still possible to be used.

SO post saying the same thing:
https://stackoverflow.com/a/50099927

But also, looking at this specific example, only strategy and remove have default values - everything else is required, so and doesn't have to be optional.

On top of that, I think we can leverage proto3 defaulting to our advantage slightly here too.

For enums, the first value is the default, so if we make SUFFIX the first value (0), then we are good to go, and for bools, the default value is false, so we also don't need optional for that.

We can likely apply this to the other configs as well. WDYT?

[iffyio]

If you use the wrapper type, it will generate a Option for the Google imported managed types.

right, also used the google types initially, what I didn't get was how I could do my own optional types like Option. tried tracing through their protos to see if I could figure it out but got lost along the way :P

But also, looking at this specific example, only strategy and remove have default values - everything else is required, so and doesn't have to be optional.

yeah not sure why they're optional, will fix

On top of that, I think we can leverage proto3 defaulting to our advantage slightly here too.

thought about this but felt it merely complicates thing further (and protobuf is already too complicated I'm finding out), didn't like that ordering would matter like that in the config for enums and e.g if default for bools is true instead we become inconsistent would rather not go that route, especially since it doesn't solve the optional problem, would rather mark optional things as optional.

[markmandel]

right, also used the google types initially, what I didn't get was how I could do my own optional types like Option. tried tracing through their protos to see if I could figure it out but got lost along the way :P

I traced the code - prost does handle the Google basic data types in a "special" way (it removes the wrapper and makes an Option<type> rather that Option<Wrapper> where the Wrapper has a value member, but you still get an Option for a value if you have your own wrapper.

So if we decide Strategy should be optional, you could do the following (written off the top of my head, so may have a couple of issue):

message CaptureBytes {
  enum StrategyEnum {
    Prefix = 0;
    Suffix = 1;
  }
  
  message Strategy {
     value StrategyEnum = 1;
  }

  Strategy strategy = 1;
  int32 size = 2;
  string metadata_key = 3;
  bool remove = 4;
}

And then the generated code is for the Strategy value will be optional out of prost (and other languages).

Re: protobuf defaulting - yeah, there's a tradeoff. Ordering in protobuf matters regardless - i.e. you can't change the numbers of variables set. At some point we'll have to handle protobuf defaulting of values somewhere - but I agree, for something like a bool, it would be weird that some boolean values were wrapped and other's weren't. So consistency is definitely a good thing 👍 and we should be consistent in our approach (so let's wrap all default values, makes sense to me).

[iffyio]

this isn't what we want either though, the above generates an option separate from the enum rather than simply option which is cumbersome with an extra layer of indirection

[markmandel]

Ideally yes, Option<StrategyEnum> would be than nicer Option<StrategyWrapper> but I think that's just the best we're going to get out of proto3 output. It's a pretty standard pattern with proto3 (hence there are standard Google wrappers for basic values).

It's a bit cumbersome on the Rust side, but it's not blocking to use. Also, we're taking that prost model and converting it into the Serde hand written model almost immediately in the code - so 99% of the time we won't be interacting with the prost generated much throughout when implementing features in a Filter -- so not seeing it as a large concern.

[iffyio]

updated ptal!

[markmandel]

Just one question about spacing. But that's all I have. I'll make it approved, so you can merge when ready.

[markmandel]

Today I learned uint32 is a proto3 data type!