fix!(codecs): Use '\0' delimiter as default stream decode framer #20778
Conversation
|
|
github-actions
bot
added
the
domain: codecs
| @@ -53,7 +59,7 @@ pub struct CharacterDelimitedDecoderOptions { | |||
|
|
|||
| impl CharacterDelimitedDecoderOptions { | |||
| /// Create a `CharacterDelimitedDecoderOptions` with a delimiter and optional max_length. | |||
| pub fn new(delimiter: u8, max_length: Option<usize>) -> Self { | |||
| pub const fn new(delimiter: u8, max_length: Option<usize>) -> Self { | |||
There was a problem hiding this comment.
Noticed this and didn't hurt
| | SerializerConfig::Json(_) | ||
| | SerializerConfig::Logfmt | ||
| | SerializerConfig::NativeJson | ||
| | SerializerConfig::RawMessage | ||
| | SerializerConfig::Text(_) => FramingConfig::NewlineDelimited, | ||
| SerializerConfig::Gelf => { |
There was a problem hiding this comment.
Huh, yeah, this does seem like an oversight. I'm trying to understand the impact of it though since the original PR did seem to resolve the issue for people 🤔
src/codecs/decoding/decoder.rs
Outdated
| @@ -138,4 +138,33 @@ mod tests { | |||
| let event = next.unwrap().0.pop().unwrap().into_log(); | |||
| assert_eq!(event.get("bar").unwrap(), &Value::from(2)); | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
| async fn gelf_streaming_decoding() { | |||
There was a problem hiding this comment.
Added this test, I don't know if its correct to place it here. Didn't find anywhere else to place it.
I'm new in this project and can't find some modules easily.
Maybe this test is more of an integration test/e2e test, but couldn't find any related in the root /tests directory. I'm missing something?
It would be nice to have something like a TCP socket integration test where we test this same behavior...As the default_stream_framing is also called in
vector/src/sources/socket/mod.rs
Line 123 in e982f66
I thought of adding a similar test like this in
vector/src/sources/socket/mod.rs
Line 391 in e982f66
In that case, I would create a TCP config setting only
decoding = DeserializerConfig::from(GelfDeserializerConfig)(and leaving the framing as None in that config) and see how the TCP initialization takes the default framer.
What are your thoughts of all of this?
|
Please, note that this is a breaking change as we are changing the default framing. Maybe someone configured a source to send '\n' delimited gelf messages and left the vector config as the default, asumming that it would be correct. In my opinion, we should adhere to the stardard and go ahead with this "breaking change" |
jorgehermo9
changed the title
jorgehermo9
changed the title
jszwedko
changed the title
There was a problem hiding this comment.
Thanks for this fix @jorgehermo9!
Do you mind adding a changelog fragment? See https://github.com/vectordotdev/vector/blob/master/changelog.d/README.md
I think the test you added is ok. I'd actually be content with just testing the the default is set correctly without testing that it actually decode with the default config and \0 delimited input since the character delimited codec is tested separately. If anything, you could add a test to lib/codecs/src/decoding/framing/character_delimited.rs that tests \0 works as a delimiter, but I don't think it is strictly necessary.
| | SerializerConfig::Json(_) | ||
| | SerializerConfig::Logfmt | ||
| | SerializerConfig::NativeJson | ||
| | SerializerConfig::RawMessage | ||
| | SerializerConfig::Text(_) => FramingConfig::NewlineDelimited, | ||
| SerializerConfig::Gelf => { |
There was a problem hiding this comment.
Huh, yeah, this does seem like an oversight. I'm trying to understand the impact of it though since the original PR did seem to resolve the issue for people 🤔
|
Thanks for the review! I'll submit the changes asap.
Right, I will narrow the scope of that test, no problem. It seemed too simple for me to just test that, but it doesn't have sense to test the \0 framing there... |
jorgehermo9
changed the title
Closes #20768.
I think that in my local environment some test fail but has nothing to do with the changes I did. Could you trigger them here and if its fails too in the CI I review what could happen?
Thanks