Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(config): Global Default Log Schemas #1769

Merged
merged 18 commits into from
Feb 14, 2020

Conversation

Hoverbear
Copy link
Contributor

@Hoverbear Hoverbear commented Feb 10, 2020

This addresses #1446.

It adds a log_schema field to the config.global. It goes into config.global since this already gets passed to the SourceConfig::build event. It also exists in the $SOURCE_source function args as this is used in some tests.

Example:

[hoverbear@obsidian:~/git/vector]$ cat test.toml 
data_dir = '/var/lib/vector/'
dns_servers = []

[log_schema]
host_key = "instance" # default "host"
message_key = "info" # default "message"
timestamp_key = "datetime" # default "timestamp"

[sources.source0]
max_length = 102400
type = 'stdin'
[sinks.sink0]
healthcheck = true
inputs = ['source0']
type = 'console'
encoding = "json"

[sinks.sink0.buffer]
type = 'memory'
max_events = 500
when_full = 'block'


[hoverbear@obsidian:~/git/vector]$ cargo run -- --config test.toml -qqq
# ....
Feb 13 04:24:30.902  INFO vector::topology: Starting sink "sink0"
Feb 13 04:24:30.902  INFO source{name=source0 type=stdin}: vector::sources::stdin: Capturing STDIN
asd
{"info":"asd","datetime":"2020-02-13T12:24:32.632275500Z","instance":"OBSIDIAN"}

@Hoverbear Hoverbear added this to the Initial schema support milestone Feb 10, 2020
@Hoverbear Hoverbear self-assigned this Feb 10, 2020
@Hoverbear Hoverbear changed the title [Draft] Global Default Log Schemas feature(config): Global Default Log Schemas [Draft] Feb 10, 2020
@binarylogic
Copy link
Contributor

The config syntax looks good! But shouldn't the resulting payload in your ouput have both the datetime and info keys since you didn't explicitly supply those as part of your input?

@Hoverbear
Copy link
Contributor Author

Ok! That clarifies it for me! I wasn't really sure if only the fields the source had configs for would be affected, or all would.

Copy link
Member

@bruceg bruceg left a comment

Choose a reason for hiding this comment

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

I have left a few questions inline.

AFAICT the kafka sink also appears to use these keys. Should it also be modified to use the new config?

src/sources/file.rs Outdated Show resolved Hide resolved
src/sources/stdin.rs Outdated Show resolved Hide resolved
src/sources/stdin.rs Outdated Show resolved Hide resolved
src/topology/config/mod.rs Outdated Show resolved Hide resolved
src/topology/config/mod.rs Outdated Show resolved Hide resolved
src/topology/config/mod.rs Outdated Show resolved Hide resolved
@Hoverbear
Copy link
Contributor Author

Thanks for this early review @bruceg :)

@Hoverbear Hoverbear requested a review from a user February 12, 2020 00:47
@Hoverbear Hoverbear requested a review from MOZGIII as a code owner February 12, 2020 00:47
@binarylogic binarylogic requested review from bruceg and removed request for MOZGIII, lukesteensen, a user and LucioFranco February 12, 2020 06:50
@binarylogic binarylogic assigned bruceg and unassigned Hoverbear Feb 12, 2020
Signed-off-by: Ana Hobden <operator@hoverbear.org>
@Hoverbear Hoverbear force-pushed the templating-add-field branch from de54b42 to 1944719 Compare February 12, 2020 21:52
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
@Hoverbear Hoverbear requested a review from Jeffail as a code owner February 13, 2020 02:04
@Hoverbear Hoverbear changed the title feature(config): Global Default Log Schemas [Draft] feature(config): Global Default Log Schemas Feb 13, 2020
@Hoverbear Hoverbear changed the title feature(config): Global Default Log Schemas feat(config): Global Default Log Schemas Feb 13, 2020
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Copy link
Contributor

@Jeffail Jeffail left a comment

Choose a reason for hiding this comment

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

LGTM, but need to initialize the globals before running unit tests as well.

Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
@Hoverbear
Copy link
Contributor Author

@Jeffail I did it the easy way. :)

// TODO: Help Rust project support before_each
// Support uninitialized schemas in tests to help our contributors.
// Don't do it in release because that is scary.
#[cfg(debug_assertions)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not 100% sure on this.

src/event/mod.rs Outdated Show resolved Hide resolved
src/event/mod.rs Outdated Show resolved Hide resolved
@@ -332,6 +335,35 @@ impl Config {
self.global.dns_servers.sort();
self.global.dns_servers.dedup();

// If the user has multiple config files, we must *merge* log schemas until we meet a
// conflict, then we are allowed to error.
let default_schema = event::LogSchema::default();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: prob could just do if with.global.log_schema = LogSchema::default()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use it later though :o

src/event/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Overall this looks fantastic, turned out much better than I thought it would :)

Signed-off-by: Ana Hobden <operator@hoverbear.org>
@@ -143,6 +143,7 @@ evmap = { version = "7", features = ["bytes"] }
logfmt = "0.0.2"
notify = "4.0.14"
once_cell = "1.3"
getset = "0.1.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you get to import your own library!

@Hoverbear
Copy link
Contributor Author

@LucioFranco Take a look again?

Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
@@ -434,6 +437,7 @@ mod test {
}

#[test]
#[ignore]
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these ignored?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed why!

Copy link
Contributor

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Just have one question about those ignored tests otherwise LGTM

Signed-off-by: Ana Hobden <operator@hoverbear.org>
Signed-off-by: Ana Hobden <operator@hoverbear.org>
@Hoverbear Hoverbear merged commit 15e69c8 into vectordotdev:master Feb 14, 2020
@binarylogic
Copy link
Contributor

Nice work on this! I just wanted to couple doc changes with it. I'll follow up with a doc change PR.

bednar pushed a commit to bonitoo-io/vector that referenced this pull request Feb 17, 2020
* Global log schemas

Signed-off-by: Ana Hobden <operator@hoverbear.org>

* Tests working and some lints cleaned.

Signed-off-by: Ana Hobden <operator@hoverbear.org>

* Fix unix tests

Signed-off-by: Ana Hobden <operator@hoverbear.org>

* Fixup of remaining unix tests

Signed-off-by: Ana Hobden <operator@hoverbear.org>

* Use correct naming.

Signed-off-by: Ana Hobden <operator@hoverbear.org>

* Add better documentation for handling conflicting log_schemas.

Signed-off-by: Ana Hobden <operator@hoverbear.org>

* fmt

Signed-off-by: Ana Hobden <operator@hoverbear.org>

* Fixup schema lookup in logdna

Signed-off-by: Ana Hobden <operator@hoverbear.org>

* Fix benches

Signed-off-by: Ana Hobden <operator@hoverbear.org>

* Fmt and fix lints

Signed-off-by: Ana Hobden <operator@hoverbear.org>

* Fix some tests that are dead on Windows.

Signed-off-by: Ana Hobden <operator@hoverbear.org>

* Docker nits

Signed-off-by: Ana Hobden <operator@hoverbear.org>

* Fix naming

Signed-off-by: Ana Hobden <operator@hoverbear.org>

* Fix a test

Signed-off-by: Ana Hobden <operator@hoverbear.org>

* Missed one!

Signed-off-by: Ana Hobden <operator@hoverbear.org>

* Unignore tests

Signed-off-by: Ana Hobden <operator@hoverbear.org>

* Reduce visiblility!

Signed-off-by: Ana Hobden <operator@hoverbear.org>
@ghost ghost mentioned this pull request Feb 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants