enhancement(sinks): Allow Encoding config to only/except list fields #1915

Hoverbear · 2020-02-24T22:29:41Z

This should fix #1448 .

So far

Things should work according to how #1448 (comment):

[sinks.my_sink]
  type = "clickhouse"
  encoding.format = "json"
  encoding.only_fields = ["timestamp", "message"]
  encoding.except_fields = ["_meta"]

Where only_fields and except_fields are mutually exclusive, only_fields should take priority. This, > of course, can be represented as a table as well:

[sinks.my_sink]
 type = "clickhouse"

 [sinks.my_sink.encoding]
   format = "json"
   only_fields = ["timestamp", "message"]
   except_fields = ["_meta"]

Please help me verify:

Each applicable sink (Those with Encoding at all) has been updated
Each sink calls encoding.validate()?
Each sink calls encoding.apply_rules(_) on all sunk events.

Each sink has the _encoding_ partial in it's .meta/_partials folder. Eg:

<%= render("_partials/_encoding.toml", namespace: "sinks.console.options") %>

To decide

I need some help from folks to determine the following:

Should this new setting be present on other sinks eg clickhouse, blackhole, or datadog?
Is there a different structure which might apply better here?
Documentation is pending

Signed-off-by: Ana Hobden <[email protected]>

binarylogic · 2020-02-25T15:10:05Z

Here's a list to make this easier:

aws_cloudwatch_logs
- call validate()
- calls encoding.apply_rules(_)
- has the _encoding_ partial in it's .meta/_partials folder
aws_kinesis_firehose
- call validate()
- calls encoding.apply_rules(_)
- has the _encoding_ partial in it's .meta/_partials folder
aws_kinesis_streams
- call validate()
- calls encoding.apply_rules(_)
- has the _encoding_ partial in it's .meta/_partials folder
aws_s3
- call validate()
- calls encoding.apply_rules(_)
- has the _encoding_ partial in it's .meta/_partials folder
clickhouse
- call validate()
- calls encoding.apply_rules(_)
- has the _encoding_ partial in it's .meta/_partials folder
console
- call validate()
- calls encoding.apply_rules(_)
- has the _encoding_ partial in it's .meta/_partials folder
datadog (Excluded: Metrics)
elasticsearch
- call validate()
- calls encoding.apply_rules(_)
- has the _encoding_ partial in it's .meta/_partials folder
file
- call validate()
- calls encoding.apply_rules(_)
- has the _encoding_ partial in it's .meta/_partials folder
gcp_cloud_storage
- call validate()
- calls encoding.apply_rules(_)
- has the _encoding_ partial in it's .meta/_partials folder
gcp_stackdriver_logging
- call validate()
- calls encoding.apply_rules(_)
- has the _encoding_ partial in it's .meta/_partials folder
http
- call validate()
- calls encoding.apply_rules(_)
- has the _encoding_ partial in it's .meta/_partials folder
humio_logs
- call validate()
- calls encoding.apply_rules(_)
- has the _encoding_ partial in it's .meta/_partials folder
kafka
- call validate()
- calls encoding.apply_rules(_)
- has the _encoding_ partial in it's .meta/_partials folder
logdna
- call validate()
- calls encoding.apply_rules(_)
- has the _encoding_ partial in it's .meta/_partials folder
loki
- call validate()
- calls encoding.apply_rules(_)
- has the _encoding_ partial in it's .meta/_partials folder
new_relic_logs
- call validate() (in http sink)
- calls encoding.apply_rules(_) (in http sink)
- has the _encoding_ partial in it's .meta/_partials folder
sematext
- call validate()
- calls encoding.apply_rules(_) (in ES sink)
- has the _encoding_ partial in it's .meta/_partials folder
socket
- call validate()
- calls encoding.apply_rules(_)
- has the _encoding_ partial in it's .meta/_partials folder
splunk_hec
- call validate()
- calls encoding.apply_rules(_)
- has the _encoding_ partial in it's .meta/_partials folder

binarylogic · 2020-02-25T15:13:21Z

Should this new setting be present on other sinks eg clickhouse, blackhole, or datadog?

I think so since one of the use cases is to discard private data before writing downstream. (ex: dropping a _private object/field). We don't need this for blackhole since it does not output data.

Signed-off-by: Ana Hobden <[email protected]>

lukesteensen

This looks like a reasonable approach 👍

The main downside seems to be requiring the validate and apply_rules calls. I think the eventual way around that is to centralize encoders so that it all can be pushed into the "library" portion of the code. That's a more invasive change to the sinks, so it makes sense to put it off for now. Could be worth opening an issue though.

Hoverbear · 2020-02-26T00:43:13Z

@lukesteensen Yeah, I think that is a good future step. Having better usability was kind of hampered by each sink having it's own encoding type in most cases. If we could unify those it'd be really nice.

lukesteensen · 2020-02-26T01:06:19Z

@Hoverbear yep, exactly. We'd need to find a good way to let each sink specify the types of encoding it supported and then delegate the actual functionality to a shared implementation.

Hoverbear · 2020-02-26T17:01:18Z

@lukesteensen One thing we can do is have an Encoding trait with a required encode function that each sink could them implement. THen the EncodingConfig<E: Encoder> can call Encoder::encode.

lukesteensen · 2020-02-26T17:03:24Z

@Hoverbear Oooo I like that. Sinks could still define their own encoders for config purposes, but maybe delegate to shared utils internally? Would also be possible to make that change incrementally.

Hoverbear · 2020-02-26T17:13:20Z

Yup! How about as a follow up to this so we don't scope creep?

lukesteensen · 2020-02-26T17:25:39Z

Sounds good!

Hoverbear · 2020-03-04T16:13:06Z

@binarylogic No, we settled on encoding.codec etc

binarylogic · 2020-03-04T16:16:33Z

Yep, I understand. So how are we handling backward compatibility? That's a very big consideration here, especially since this is a popular option.

LucioFranco · 2020-03-04T16:19:08Z

@binarylogic it looks like using encoding = "string" should still work thus maintain backwards compat. This supports both methods.

binarylogic · 2020-03-04T16:21:46Z

Sounds good,

Signed-off-by: Lucio Franco <[email protected]>

LucioFranco

This looks really really good! Left some comments in line, let me know if you have any questions :)

src/sinks/aws_cloudwatch_logs/mod.rs

LucioFranco · 2020-03-04T16:29:51Z

src/sinks/aws_kinesis_firehose.rs

 }

 #[typetag::serde(name = "aws_kinesis_firehose")]
 impl SinkConfig for KinesisFirehoseSinkConfig {
    fn build(&self, cx: SinkContext) -> crate::Result<(super::RouterSink, super::Healthcheck)> {
        let config = self.clone();
+        config.encoding.validate()?;


since it looks like we call this on each build we could just nest this into our de impl for EncodingConfig?

src/sinks/aws_cloudwatch_logs/mod.rs

LucioFranco · 2020-03-04T16:31:09Z

src/sinks/clickhouse.rs

-    pub encoding: EncodingConfig,
+    #[serde(
+        deserialize_with = "EncodingConfigWithDefault::from_deserializer",
+        skip_serializing_if = "skip_serializing_if_default",


Maybe I'm missing something but why do some sinks have this and others don't? My assumption is that this allows a "default" encoding?

To avoid further bikeshedding this should in theory be possible to do within a custom impl but happy to leave that out for now.

We want to make sure a user never has to see encoding.format if there is only one option. So this makes sure we skip this field when it's not required in vector generate calls

LucioFranco · 2020-03-04T16:33:35Z

src/sinks/humio_logs.rs

 #[typetag::serde(name = "humio_logs")]
 impl SinkConfig for HumioLogsConfig {
    fn build(&self, cx: SinkContext) -> crate::Result<(super::RouterSink, super::Healthcheck)> {
+        if self.encoding.codec != Encoding::Json {


I wonder if there is a better way around this, in theory could we just implement a custom encoding enum here that doesn't have Json?

Yeah I don't think this is the nicest way either but it's very straightforward

The one thing that worries me is I've seen serde sometimes enumerate all the possible options but in this case a user would see json as being available but would fail when you configure the sink. Just something we should check.

Yeah, hm... The only way to fix that would be to give this a custom encoding type and transmute it

Why not write a custom enum here that doesn't include json?

src/sinks/util/encoding.rs

Signed-off-by: Ana Hobden <[email protected]>

binarylogic · 2020-03-04T17:48:57Z

Just to verify: Do we have a behavior test with the root level encoding (legacy) option set? If not, we should add one or more.

Hoverbear · 2020-03-04T18:06:00Z

@binarylogic They are here: https://github.com/timberio/vector/pull/1915/files#diff-6c009d8ad9b35057ba119e7374c4abd9R429-R443

binarylogic · 2020-03-04T18:30:08Z

That link doesn't go anywhere :(

LucioFranco · 2020-03-04T18:45:37Z

gh links for lines in PRs have been broken for me for a while now...

Signed-off-by: Ana Hobden <[email protected]>

Hoverbear · 2020-03-04T21:37:27Z

@LucioFranco Our powers combined, this seems ready once the tests pass.

LucioFranco

Fantastic work! 🔥

src/sinks/file/mod.rs

src/sinks/new_relic_logs.rs

binarylogic · 2020-03-04T23:42:40Z

I'm slightly concerned this is not correct given:

The back and forth around defaults, enumerations, error messages, and so on.
The fact that chore(config): Make encoding non-optional #894 missed a number of sinks.

How can we simply test for this? It seems silly to test for something so trivial, but I feel like we need to at this point. Should we create configs and lint them? Maybe fuzz testing?

Hoverbear · 2020-03-05T00:03:11Z

@binarylogic Let's not stack anything more on this PR, we already chose to unnecessarily stack the fixes for #894 in this. (Which I think was a good idea!)

So, please create a separate issue! I'd love to share my thoughts there!

Prior to merging I'll be doing some manual user acceptance testing. Our exiting unit tests already serve as a fairly good configuration test bank. Note how our clickhouse tests tend to use configuration fragments. I think this is a good way to do things since it lets us safely do these changes and catch mistakes. It'd be nice if we trended towards doing this more over using structs.

Hoverbear · 2020-03-05T00:10:39Z

I'm going to test this out locally. I've identified a few cases in particular I'm checking:

Check sinks with a single encoding type that don't get into'd don't print it out with cargo generate.
```
vector generate stdin//elasticsearch
# Ensure `encoding.codec` doesn't exist. Is runnable.
```
Check sinks with a single encoding type that gets into'd don't print it out with cargo generate.
```
vector generate stdin//humio_logs
# Ensure `encoding.codec` doesn't exist. Is runnable.
```

Check sinks with multiple encodings and no default don't default to one.

vector generate stdin//file
# Ensure `encoding.codec` doesn't exist, and that we get an error on run until a user writes it in.

Check sinks with multiple encodings and a default do default to that one.

vector generate stdin//aws_s3
# Ensure `encoding.codec` doesn't exist. Is runnable.

Signed-off-by: Ana Hobden <[email protected]>

Hoverbear · 2020-03-05T02:33:32Z

Okay those examples work (Except #1987 ) so I'm going to merge this under the impression that any remaining fixes will be (very) minor and will most likely be related to setting encoding.only_fields without setting encoding.codec, (So, we missed the skip_if_default serde flag) which is a very straightforward workaround.)

Hoverbear added 4 commits February 24, 2020 13:18

Initial EncodingConfig

689d4ac

Signed-off-by: Ana Hobden <[email protected]>

Improve EncodingConfig

6cf4cae

Signed-off-by: Ana Hobden <[email protected]>

Update other sinks

caec215

Signed-off-by: Ana Hobden <[email protected]>

Cleanup

d645f98

Signed-off-by: Ana Hobden <[email protected]>

Hoverbear added the type: new feature label Feb 24, 2020

Hoverbear requested a review from LucioFranco February 24, 2020 22:29

Hoverbear requested a review from lukesteensen as a code owner February 24, 2020 22:29

Hoverbear self-assigned this Feb 24, 2020

Hoverbear added the domain: sinks Anything related to the Vector's sinks label Feb 24, 2020

Hoverbear added 2 commits February 24, 2020 14:49

Fixup splunk

75491c4

Signed-off-by: Ana Hobden <[email protected]>

Fix kinesis

855f6e8

Signed-off-by: Ana Hobden <[email protected]>

Hoverbear changed the title ~~feat: Allow Encoding config to black or whitelist fields~~ feat: Allow Encoding config to black or whitelist fields (WIP) Feb 25, 2020

wip checkpoint

9c5da70

Signed-off-by: Ana Hobden <[email protected]>

Hoverbear requested a review from binarylogic as a code owner February 25, 2020 15:01

binarylogic changed the title ~~feat: Allow Encoding config to black or whitelist fields (WIP)~~ enhancement(sinks): Allow Encoding config to black or whitelist fields (WIP) Feb 25, 2020

Additional implementations

55a4741

Signed-off-by: Ana Hobden <[email protected]>

Hoverbear requested a review from a user February 25, 2020 19:52

Hoverbear requested a review from bruceg as a code owner February 25, 2020 19:52

Hoverbear added 3 commits February 25, 2020 11:53

Play with schemas a bit more

5a256f6

Signed-off-by: Ana Hobden <[email protected]>

Merge branch 'master' into encoding-config

a536c4a

Support timestamp_format

9d9ad4c

Signed-off-by: Ana Hobden <[email protected]>

lukesteensen reviewed Feb 25, 2020

View reviewed changes

remove annoying attributes

5c1769d

Signed-off-by: Lucio Franco <[email protected]>

LucioFranco reviewed Mar 4, 2020

View reviewed changes

Hoverbear added 3 commits March 4, 2020 08:44

Change defaults

00da854

Signed-off-by: Ana Hobden <[email protected]>

Move validate into deser

3bfb77d

Signed-off-by: Ana Hobden <[email protected]>

Fixup a test

a6cb1e2

Signed-off-by: Ana Hobden <[email protected]>

Hoverbear added 3 commits March 4, 2020 12:08

Merge branch 'master' into encoding-config

402e8b3

Fixup InnerWithDefaults

ccb502c

Signed-off-by: Ana Hobden <[email protected]>

remove Inner impls

55346a5

Signed-off-by: Ana Hobden <[email protected]>

LucioFranco approved these changes Mar 4, 2020

View reviewed changes

src/sinks/file/mod.rs Outdated Show resolved Hide resolved

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

Rejigger encodings to use into better

81a49d8

Signed-off-by: Ana Hobden <[email protected]>

Hoverbear merged commit 2396860 into vectordotdev:master Mar 5, 2020

This was referenced Mar 11, 2020

feat(new sink): Initial papertrail sink #1835

Merged

feat(new sink): Initial basic datadog_logs sink #1832

Merged

binarylogic mentioned this pull request Apr 22, 2020

New log_schema.meta_key option as a new metadata convention #2415

Closed

binarylogic added type: feature A value-adding code addition that introduce new functionality. and removed type: new feature labels Jun 16, 2020

binarylogic mentioned this pull request Nov 23, 2020

Make encoding.codec non-optional or default to json/ndjson #5196

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enhancement(sinks): Allow Encoding config to only/except list fields #1915

enhancement(sinks): Allow Encoding config to only/except list fields #1915

Hoverbear commented Feb 24, 2020 •

edited

Loading

binarylogic commented Feb 25, 2020 •

edited by Hoverbear

Loading

binarylogic commented Feb 25, 2020 •

edited

Loading

lukesteensen left a comment

Hoverbear commented Feb 26, 2020 •

edited

Loading

lukesteensen commented Feb 26, 2020

Hoverbear commented Feb 26, 2020

lukesteensen commented Feb 26, 2020

Hoverbear commented Feb 26, 2020

lukesteensen commented Feb 26, 2020

Hoverbear commented Mar 4, 2020

binarylogic commented Mar 4, 2020

LucioFranco commented Mar 4, 2020

binarylogic commented Mar 4, 2020

LucioFranco left a comment

LucioFranco Mar 4, 2020

LucioFranco Mar 4, 2020

Hoverbear Mar 4, 2020

LucioFranco Mar 4, 2020

Hoverbear Mar 4, 2020

LucioFranco Mar 4, 2020

Hoverbear Mar 4, 2020 •

edited

Loading

LucioFranco Mar 4, 2020

Hoverbear Mar 4, 2020

binarylogic commented Mar 4, 2020

Hoverbear commented Mar 4, 2020

binarylogic commented Mar 4, 2020

LucioFranco commented Mar 4, 2020

Hoverbear commented Mar 4, 2020

LucioFranco left a comment

binarylogic commented Mar 4, 2020

Hoverbear commented Mar 5, 2020

Hoverbear commented Mar 5, 2020

Hoverbear commented Mar 5, 2020

enhancement(sinks): Allow Encoding config to only/except list fields #1915

enhancement(sinks): Allow Encoding config to only/except list fields #1915

Conversation

Hoverbear commented Feb 24, 2020 • edited Loading

So far

To decide

binarylogic commented Feb 25, 2020 • edited by Hoverbear Loading

binarylogic commented Feb 25, 2020 • edited Loading

lukesteensen left a comment

Choose a reason for hiding this comment

Hoverbear commented Feb 26, 2020 • edited Loading

lukesteensen commented Feb 26, 2020

Hoverbear commented Feb 26, 2020

lukesteensen commented Feb 26, 2020

Hoverbear commented Feb 26, 2020

lukesteensen commented Feb 26, 2020

Hoverbear commented Mar 4, 2020

binarylogic commented Mar 4, 2020

LucioFranco commented Mar 4, 2020

binarylogic commented Mar 4, 2020

LucioFranco 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

Hoverbear Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

binarylogic commented Mar 4, 2020

Hoverbear commented Mar 4, 2020

binarylogic commented Mar 4, 2020

LucioFranco commented Mar 4, 2020

Hoverbear commented Mar 4, 2020

LucioFranco left a comment

Choose a reason for hiding this comment

binarylogic commented Mar 4, 2020

Hoverbear commented Mar 5, 2020

Hoverbear commented Mar 5, 2020

Hoverbear commented Mar 5, 2020

Hoverbear commented Feb 24, 2020 •

edited

Loading

binarylogic commented Feb 25, 2020 •

edited by Hoverbear

Loading

binarylogic commented Feb 25, 2020 •

edited

Loading

Hoverbear commented Feb 26, 2020 •

edited

Loading

Hoverbear Mar 4, 2020 •

edited

Loading