chore(codecs): Add RFC for "Framing and Codecs - Sinks"

[pablosichert]

Closes #8621.

Rendered version 👀.

[netlify]

✔️ Deploy Preview for vector-project canceled.

🔨 Explore the source changes: f820c10

🔍 Inspect the deploy log: https://app.netlify.com/sites/vector-project/deploys/61dd84ebb4122d0007f09cb2

[bits-bot]

All committers have signed the CLA.

[tobz]

The one question that I think needs to be answered here is: will the schema work land before this happens? If it doesn't, then we probably need a way handle parsing these out of the existing encoding configuration field. We could deprecate setting them via encoding at the same time, but we would have to provide a deprecation window regardless.

[lukesteensen]

Yeah, I would suggest we continue to support the current config interface for now and focus on organizing the internal implementation for now. Then once both this work and the schema work is further along we can tackle changing the external API with a bit more confidence.

[pablosichert]

Sounds like a good plan 👍 Should be able to support existing config options without carrying over too much baggage.

[pablosichert]

I came up with a wrapper that is able to apply the existing transformation behavior to the new encoding options while being fully backwards compatible to the old sink-specific encoding options: #10253.

For usage, have a look at #10684.

[pablosichert]

Clarified in 21f9c36.

[tobz]

One thing I'd like to see here is if we can actually make SerializerConfig/FramingConfig provide their outputs generically. For example:

pub trait SerializerConfig {
    type Serializer;
    
    fn build(&self) -> crate::Result<Self::Serializer>;
}

pub trait FramingConfig {
    type Framer;
    
    fn build(&self) -> crate::Result<Self::Framer>;
}

pub struct Encoder<S, F> {
    serializer: S,
    framer: F,
}

This is sort of an implementation detail, but we should avoid boxing individual components where possible, and prefer to box at a higher level. It might even be possible to entirely avoid boxing. I'm bringing this up here on the off-chance that not using boxed inner fields would meaningfully change how we have to approach the implementation.

Since codecs touch every single event entering and exiting Vector, we should take care to avoid boxing where possible.

[pablosichert]

Hmm, this is a very interesting point.

Since the source side is structured very similarly, I tried to apply your suggestion to the FramingConfig and ParserConfig.

It fails to compile since dyn_clone::clone_trait_object needs to specify the associated type. Cloning a decoder/encoder is a rather common operation, it happens for each message or stream to reset the codec state.

I'm not sure if it's possible to circumvent this without having a Box on the inner type, but I'd be happy for any suggestions.

[lukesteensen]

Yeah, I would suggest we continue to support the current config interface for now and focus on organizing the internal implementation for now. Then once both this work and the schema work is further along we can tackle changing the external API with a bit more confidence.

[pablosichert]

Thanks to @fuchsnj for noting that all sinks using EncodingConfig* reshape event with apply_rules via the generic implementation in

vector/src/sinks/util/encoding/mod.rs

Lines 245 to 257 in 8ce0e67

	impl<E, T> Encoder<T> for E
	where
	E: EncodingConfiguration,
	E::Codec: Encoder<T>,
	T: VisitLogMut,
	{
	fn encode_input(&self, mut input: T, writer: &mut dyn io::Write) -> io::Result<usize> {
	input.visit_logs_mut(|log| {
	self.apply_rules(log);
	});
	self.codec().encode_input(input, writer)
	}
	}

[lukesteensen]

Do you imagine this will work in the same way for sinks that don't use one of the pieces (e.g. framing)?

It seems like sinks that entirely do their own serialization and framing (e.g. datadog_logs) would have an easy enough time opting out entirely, but would something like kafka have to pay some performance/complexity overhead of dealing with the possibility of a framer even though that's built into the protocol?

[pablosichert]

Hm, I see your point. The Kafka sink would still be able to add a SerializerConfig only, to build the Serializer without wrapping it in an Encoder.

Otherwise could be worth to do some micro benchmarks for how much of an impact going through one no-op framing operation has. Might not be significant, as the cost only occurs once per frame.

[pablosichert]

Clarified in d7c9cbf.

[pablosichert]

Modified the RFC to reflect the changes to the Encoder/Serializer/Framer discussed in #10108 (comment).