enhancement!: Remove default `encoding.codec` where appropriate #5281

fanatid · 2020-11-29T15:30:47Z

Closes #5196

Affected components (remove text value by default, not backward compatible):

aws_s3, text -> null
file, text -> null
humio, json -> null
kafka, text -> null
nats, text -> null
new_relic_logs, json -> null
pulsar, text -> null
splunk_hec, text -> null

Signed-off-by: Kirill Fomichev <[email protected]>

binarylogic · 2020-11-29T16:58:09Z

@kirillt shouldn't this include .cue changes?

fanatid · 2020-11-29T17:22:28Z

@binarylogic yep, I'll need to update the docs too

Signed-off-by: Kirill Fomichev <[email protected]>

Hoverbear · 2020-11-30T20:08:38Z

src/sinks/util/encoding/encodings.rs

+#[derive(Clone, Copy, Debug, Derivative, Deserialize, Eq, PartialEq, Serialize)]
+#[derivative(Default)]
+#[serde(rename_all = "snake_case")]
+pub enum EncodingText {


Hmmm I guess the alternative here would be to have a single Encoding type with no default, then make each component implement a serde default function? It might adapt better as we add things like #5021

👍 I think I like this better too.

Hoverbear

There is nothing necessarily wrong with this implementation persay, it's just very inelegant and it will become quite burdensome as our system develops and adds support for more encoding types.

I think this is fine to merge, but if you have a different implementation idea for accomplishing this, I'd encourage you to investigate it. :)

binarylogic · 2020-11-30T20:16:11Z

I tend to agree with @Hoverbear. I appreciate the effort, but coupling the various types in this way is likely to be difficult to manage in the future. We plan to expand support in the future.

Signed-off-by: Kirill Fomichev <[email protected]>

fanatid · 2020-12-01T11:04:13Z

Yeah, I think now that extend enums will be harder. I returned the original Encoding enums back.

src/sources/splunk_hec.rs

src/sinks/splunk_hec.rs

Hoverbear

I noted some spots we might prefer a default in a generated config, the rest looks good. :)

Signed-off-by: Kirill Fomichev <[email protected]>

jszwedko

One question below, but otherwise this looks good to me!

It is making me wonder if we even want to default the Humio and New Relic sinks though. Should we just require it there as well for consistency?

src/sinks/util/buffer/compression.rs

…fault

fanatid · 2020-12-08T10:39:59Z

It is making me wonder if we even want to default the Humio and New Relic sinks though. Should we just require it there as well for consistency?

New Relic has only one encoding (json) so I think it's okay to have it by default. https://github.com/timberio/vector/blob/efef1185c743ed6ad2ceed82694af5abd4210859/src/sinks/new_relic_logs.rs#L70-L73

Why we need to remove the default value in Humio (json)?

jszwedko · 2020-12-08T16:15:13Z

It is making me wonder if we even want to default the Humio and New Relic sinks though. Should we just require it there as well for consistency?

New Relic has only one encoding (json) so I think it's okay to have it by default.

https://github.com/timberio/vector/blob/efef1185c743ed6ad2ceed82694af5abd4210859/src/sinks/new_relic_logs.rs#L70-L73

Why we need to remove the default value in Humio (json)?

I was just thinking, as a user of vector, it would be nice to have the sinks be consistent in their requirement of encoding.codec than to have exceptions for only one or two sinks.

For New Relic, I think we could easily add support for text encoding by setting message in the payload (https://docs.newrelic.com/docs/logs/log-management/log-api/introduction-log-api) to the message field in vector rather than encoding the event as JSON. Again, I think this consistency across sinks would make vector easier to use.

fanatid · 2020-12-08T17:58:13Z

@binarylogic what do you think about Humio and New Relic? Should we change default encoding?

binarylogic · 2020-12-08T18:03:44Z

They should default to JSON. We should aim to reduce decisions imposed on the user, and I cannot think of a reason JSON should not be the default for those sinks (and most sinks).

jszwedko · 2020-12-08T18:11:00Z

They should default to JSON. We should aim to reduce decisions imposed on the user, and I cannot think of a reason JSON should not be the default for those sinks (and most sinks).

I don't disagree with this, but that is a different direction than this PR is taking. If we want to prefer to default to JSON, we should default to JSON in this PR everywhere feasible rather than removing default encodings.

binarylogic · 2020-12-08T18:20:40Z

I agree, we should default to JSON where possible. I can't think of a scenario where we would not default to JSON if JSON is supported by the sink.

lukesteensen · 2020-12-09T02:50:40Z

I agree, we should default to JSON where possible. I can't think of a scenario where we would not default to JSON if JSON is supported by the sink.

We've had this discussion many times. There are situations like syslog -> syslog and kafka -> kafka where the user would not necessarily expect their message to be wrapped in a JSON envelope before being forwarded. For that reason, we decided to avoid setting a default on sinks that support multiple encodings.

Humio, New Relic, and similar will often have their own JSON wrappers, but that doesn't mean it always makes sense to wrap each individual event in further JSON. For example, consider a file -> humio config with no parsing. With a "raw" encoding, each event will look (roughly) like this:

{
  "event": "I am a raw log line from the input file",
  "fields": [],
  "host": "bens-laptop.local",
  "time": "2006-08-14T02:34:56-06:00"
 }

If we default to JSON, it'll look like the following:

{
  "event": {
    "message": "I am a raw log line from the input file",
    "timestamp": "2006-08-14T02:34:56-06:00",
    "host": "bens-laptop.local"
  },
  "fields": [],
  "host": "bens-laptop.local",
  "time": "2006-08-14T02:34:56-06:00"
}

As you can see, in a pipeline with no real parsing or enrichment, the end result of encoding with JSON is a potentially confusing duplication of metadata that's already handled by the sink's normal protocol.

Signed-off-by: Kirill Fomichev <[email protected]>

fanatid · 2020-12-09T10:35:42Z

Default encoding in humio / new-relic-logs removed.

jszwedko

Thanks @fanatid !

It looks like the loki sink still has a default, but multiple options, should we remove that one too? That was the only other one I could find.

As mentioned in my in-line comment, I think we should add support for text encoding for New Relic as well.

jszwedko · 2020-12-09T15:51:30Z

src/sinks/new_relic_logs.rs

 pub enum Encoding {
-    #[derivative(Default)]
    Json,


I think we should add Text here as an encoding option.

Are you okay with adding in another PR? This will not be breaking change because we already removed Default for Encoding.

👍 a separate PR is fine, but it'd be nice to do it before this change is released in a new version as it'd be confusing to require users to specify the encoding.codec option when there is only one valid value.

jszwedko · 2020-12-09T15:52:50Z

src/sinks/util/encoding/config.rs

@@ -67,6 +67,22 @@ where
    }
 }

+#[cfg(any(feature = "sinks-new_relic_logs", feature = "sinks-humio"))]


I'm a little confused why this type only is used by these two sinks and not the others. Should it be?

Yes, it's should be here. Only used in New Relic / Humio because we need to convert EncodingConfig<sink1::Encoding> to EncodingConfig<sink2::Encoding>.

Ah! Gotcha, I see.

Signed-off-by: Kirill Fomichev <[email protected]>

fanatid · 2020-12-09T16:25:30Z

@jszwedko default in loki removed.

jszwedko · 2020-12-09T16:53:41Z

The Windows build failure looks related:

---- sinks::humio::logs::tests::humio_valid_time_field stdout ----
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error { inner: ErrorInner { kind: Custom, line: Some(0), col: 0, at: Some(0), message: "missing field `encoding`", key: [] } }', src\sinks\humio\logs.rs:123:10

Whenever CI is green this looks good to me!

Signed-off-by: Kirill Fomichev <[email protected]>

binarylogic · 2020-12-10T15:20:38Z

Nice work, thanks @kirillt .

This is the only supported encoding for the New Relic sink. There was some discussion about this in #5281 when the default was dropped. It was dropped as we thought `text` was also a valid encoding, that we would be adding shortly, in which case it would make sense to make the user choose; however, I found that `text` is not a valid encoding for the New Relic logs API (https://docs.newrelic.com/docs/logs/log-management/log-api/introduction-log-api/). Also document the undocumented region field. Signed-off-by: Jesse Szwedko <[email protected]>

fanatid added 6 commits November 29, 2020 17:34

remove not used Into impl

182caf3

Signed-off-by: Kirill Fomichev <[email protected]>

remove duplicated skip_serializing_if_default

7824477

Signed-off-by: Kirill Fomichev <[email protected]>

use skip_serializing_if_default in EncodingConfig for consistency

4bee15b

Signed-off-by: Kirill Fomichev <[email protected]>

add encodings

fddc54b

Signed-off-by: Kirill Fomichev <[email protected]>

define fn only for features

d6f4d2b

Signed-off-by: Kirill Fomichev <[email protected]>

remove custom Encoding from sources-http

5b6cb0f

Signed-off-by: Kirill Fomichev <[email protected]>

fanatid added the type: enhancement A value-adding code change that enhances its existing functionality. label Nov 29, 2020

fanatid added this to the 2020-11-23: Pseudo-chitin armor milestone Nov 29, 2020

fanatid requested review from Hoverbear and binarylogic November 29, 2020 15:30

fanatid self-assigned this Nov 29, 2020

fanatid requested review from bruceg, jszwedko, ktff and lukesteensen as code owners November 29, 2020 15:30

fanatid removed request for bruceg, ktff and jszwedko November 29, 2020 15:31

fanatid added 4 commits November 29, 2020 20:33

clippy fix

1225f30

Signed-off-by: Kirill Fomichev <[email protected]>

fix Compression deserialization

ee985ab

Signed-off-by: Kirill Fomichev <[email protected]>

fix shutdown test

b4fc074

Signed-off-by: Kirill Fomichev <[email protected]>

fix docs

2645669

Signed-off-by: Kirill Fomichev <[email protected]>

Hoverbear reviewed Nov 30, 2020

View reviewed changes

fanatid added 2 commits December 1, 2020 13:39

return enums back

22fd90e

Signed-off-by: Kirill Fomichev <[email protected]>

revert not required changes

7c9f5c7

Signed-off-by: Kirill Fomichev <[email protected]>

Hoverbear reviewed Dec 1, 2020

View reviewed changes

src/sources/splunk_hec.rs Outdated Show resolved Hide resolved

Hoverbear reviewed Dec 1, 2020

View reviewed changes

src/sinks/splunk_hec.rs Outdated Show resolved Hide resolved

Hoverbear reviewed Dec 1, 2020

View reviewed changes

src/sinks/splunk_hec.rs Outdated Show resolved Hide resolved

Hoverbear approved these changes Dec 1, 2020

View reviewed changes

add Ana changes

455e49c

Signed-off-by: Kirill Fomichev <[email protected]>

jszwedko approved these changes Dec 4, 2020

View reviewed changes

src/sinks/util/buffer/compression.rs Outdated Show resolved Hide resolved

fanatid mentioned this pull request Dec 7, 2020

fix(config): fixing Compression parsing with tests #5429

Merged

Merge remote-tracking branch 'upstream/master' into encoding-codec-de…

efef118

…fault

remove default encoding for humio/new-relic-logs

1bbd146

Signed-off-by: Kirill Fomichev <[email protected]>

fanatid changed the title ~~enhancement!: Remove text as default encoding.codec where appropriate~~ enhancement!: Remove default encoding.codec where appropriate Dec 9, 2020

jszwedko reviewed Dec 9, 2020

View reviewed changes

remove default for loki

93d6ae9

Signed-off-by: Kirill Fomichev <[email protected]>

jszwedko approved these changes Dec 9, 2020

View reviewed changes

fix tests

076e7db

Signed-off-by: Kirill Fomichev <[email protected]>

fanatid merged commit 3a48a0a into master Dec 10, 2020

fanatid deleted the encoding-codec-default branch December 10, 2020 08:59

jszwedko mentioned this pull request Jun 15, 2021

fix(new_relic_logs sink): Default encoding field to json #7870

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhancement!: Remove default `encoding.codec` where appropriate #5281

enhancement!: Remove default `encoding.codec` where appropriate #5281

fanatid commented Nov 29, 2020 •

edited

Loading

binarylogic commented Nov 29, 2020

fanatid commented Nov 29, 2020

Hoverbear Nov 30, 2020

jszwedko Dec 1, 2020

Hoverbear left a comment

binarylogic commented Nov 30, 2020

fanatid commented Dec 1, 2020

Hoverbear left a comment

jszwedko left a comment

fanatid commented Dec 8, 2020

jszwedko commented Dec 8, 2020

fanatid commented Dec 8, 2020

binarylogic commented Dec 8, 2020 •

edited

Loading

jszwedko commented Dec 8, 2020

binarylogic commented Dec 8, 2020

lukesteensen commented Dec 9, 2020

fanatid commented Dec 9, 2020

jszwedko left a comment

jszwedko Dec 9, 2020

fanatid Dec 9, 2020

jszwedko Dec 9, 2020

jszwedko Dec 9, 2020

fanatid Dec 9, 2020

jszwedko Dec 9, 2020

fanatid commented Dec 9, 2020

jszwedko commented Dec 9, 2020

binarylogic commented Dec 10, 2020

enhancement!: Remove default encoding.codec where appropriate #5281

enhancement!: Remove default encoding.codec where appropriate #5281

Conversation

fanatid commented Nov 29, 2020 • edited Loading

binarylogic commented Nov 29, 2020

fanatid commented Nov 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hoverbear left a comment

Choose a reason for hiding this comment

binarylogic commented Nov 30, 2020

fanatid commented Dec 1, 2020

Hoverbear left a comment

Choose a reason for hiding this comment

jszwedko left a comment

Choose a reason for hiding this comment

fanatid commented Dec 8, 2020

jszwedko commented Dec 8, 2020

fanatid commented Dec 8, 2020

binarylogic commented Dec 8, 2020 • edited Loading

jszwedko commented Dec 8, 2020

binarylogic commented Dec 8, 2020

lukesteensen commented Dec 9, 2020

fanatid commented Dec 9, 2020

jszwedko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanatid commented Dec 9, 2020

jszwedko commented Dec 9, 2020

binarylogic commented Dec 10, 2020

enhancement!: Remove default `encoding.codec` where appropriate #5281

enhancement!: Remove default `encoding.codec` where appropriate #5281

fanatid commented Nov 29, 2020 •

edited

Loading

binarylogic commented Dec 8, 2020 •

edited

Loading