Skip to content
Merged
37 changes: 37 additions & 0 deletions changelog.d/20768-default_gelf_stream_framing.breaking.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
Now the GELF codec with stream-based sources uses null byte (`\0`) by default as messages delimiter instead of newline (`\n`) character. This better matches GELF server behavior.

### Configuration changes

In order to maintain the previous behavior, you must set the `framing.method` option to the `character_delimited` method and the `framing.character_delimited.delimiter` option to `\n` when using GELF codec with stream-based sources.

### Example configuration change for socket source

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for including the example for using the previous default.

Small nit that we try to use YAML rather than TOML as the default these days.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!


#### Previous

```yaml
sources:
my_source_id:
type: "socket"
address: "0.0.0.0:9000"
mode: "tcp"
decoding:
codec: "gelf"
```

#### Current

```yaml
sources:
my_source_id:
type: "socket"
address: "0.0.0.0:9000"
mode: "tcp"
decoding:
codec: "gelf"
framing:
method: "character_delimited"
character_delimited:
delimiter: "\n"
```

authors: jorgehermo9
8 changes: 7 additions & 1 deletion lib/codecs/src/decoding/framing/character_delimited.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ pub struct CharacterDelimitedDecoderConfig {
}

impl CharacterDelimitedDecoderConfig {
/// Creates a `CharacterDelimitedDecoderConfig` with the specified delimiter and default max length.
pub const fn new(delimiter: u8) -> Self {
Self {
character_delimited: CharacterDelimitedDecoderOptions::new(delimiter, None),
}
}
/// Build the `CharacterDelimitedDecoder` from this configuration.
pub const fn build(&self) -> CharacterDelimitedDecoder {
if let Some(max_length) = self.character_delimited.max_length {
Expand Down Expand Up @@ -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 {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed this and didn't hurt

Self {
delimiter,
max_length,
Expand Down
24 changes: 23 additions & 1 deletion lib/codecs/src/decoding/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -334,14 +334,16 @@ impl DeserializerConfig {
DeserializerConfig::Native => FramingConfig::LengthDelimited(Default::default()),
DeserializerConfig::Bytes
| DeserializerConfig::Json(_)
| DeserializerConfig::Gelf(_)
| DeserializerConfig::NativeJson(_) => {
FramingConfig::NewlineDelimited(Default::default())
}
DeserializerConfig::Protobuf(_) => FramingConfig::Bytes,
#[cfg(feature = "syslog")]
DeserializerConfig::Syslog(_) => FramingConfig::NewlineDelimited(Default::default()),
DeserializerConfig::Vrl(_) => FramingConfig::Bytes,
DeserializerConfig::Gelf(_) => {
FramingConfig::CharacterDelimited(CharacterDelimitedDecoderConfig::new(0))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about this these days... I think that GELF Http maybe uses newline delimiter? I can't find anything related to streaming gelf in Http here https://go2docs.graylog.org/5-0/getting_in_log_data/gelf.html, but maybe someone will rely on that for http sources also and this will be a breaking change in those cases too, not only with TCP.

If you are ok with that, I will include that breaking changes and the "fix" (manually setting the newline delimiter in framing.method) in the changelog fragment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I haven't heard of anyone using GELF over HTTP, but it seems plausible. TCP seems to be the dominant transport so I think it makes sense to configure the defaults to work well with that.

}
}
}

Expand Down Expand Up @@ -467,3 +469,23 @@ impl format::Deserializer for Deserializer {
}
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn gelf_stream_default_framing_is_null_delimited() {
let deserializer_config = DeserializerConfig::from(GelfDeserializerConfig::default());
let framing_config = deserializer_config.default_stream_framing();
matches!(
framing_config,
FramingConfig::CharacterDelimited(CharacterDelimitedDecoderConfig {
character_delimited: CharacterDelimitedDecoderOptions {
delimiter: 0,
max_length: None,
}
})
);
}
}
4 changes: 3 additions & 1 deletion lib/codecs/src/encoding/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -362,12 +362,14 @@ impl SerializerConfig {
FramingConfig::LengthDelimited(LengthDelimitedEncoderConfig::default())
}
SerializerConfig::Csv(_)
| SerializerConfig::Gelf
| SerializerConfig::Json(_)
| SerializerConfig::Logfmt
| SerializerConfig::NativeJson
| SerializerConfig::RawMessage
| SerializerConfig::Text(_) => FramingConfig::NewlineDelimited,
SerializerConfig::Gelf => {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to be consistent with the default_stream_framing of the DeserializerConfig. Noticed that this change was missing in #18008. Changed it here, but tell me if you want this PR to not do anything out of scope from #20768.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the changed struct was EncodingConfigWithFraming (#18419).

It seems that a few places relies on EncodingConfigWithFraming to get this default behaviour, and other places uses this method SerializerConfig::default_stream_framing. For example, see this line

encoder_framing_to_decoding_framer(config.config().default_stream_framing()),

Where we can see its relying on SerializerConfig::default_stream_framing in order to get the default framing for a given encoder.

There are a lot of sinks that relies on EncodingConfigWithFraming having a Option in order to calculate that default (https://github.com/search?q=repo%3Avectordotdev%2Fvector%20EncodingConfigWithFraming&type=code)

(None, Serializer::Gelf(_)) => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, gotcha, thanks for looking into that.

FramingConfig::CharacterDelimited(CharacterDelimitedEncoderConfig::new(0))
}
}
}

Expand Down