Skip to content

fix(config)!: require encoding for console and file sinks#1033

Merged
lukesteensen merged 7 commits intomasterfrom
guide-issues
Oct 21, 2019
Merged

fix(config)!: require encoding for console and file sinks#1033
lukesteensen merged 7 commits intomasterfrom
guide-issues

Conversation

@lukesteensen
Copy link
Copy Markdown
Member

@lukesteensen lukesteensen commented Oct 15, 2019

Closes #1031

There were a handful of small-ish issues preventing things from working the way our coercer guide claims they should:

  1. The example timestamp format string didn't match the example data
  2. We weren't logging the failure to convert types from (1) at a level that's visible by default
  3. We forgot the console and file sinks when fixing Update sink encoding to be non optional #806, so even when (1) and (2) were addressed the console sink wouldn't output the event
  4. There were a couple of typos in the config metadata that gave them incorrect docs

This is a breaking change for both the console and file sinks, since the encoding field is no longer optional.

Upgrade guide

  1. If you're using the console sink add the encoding option:

    [sinks.print]
      type = "console"
      encoding = "json" # or text
  2. If you're using the file sink add the encoding option:

    [sinks.file_out]
      type = "file"
      encoding = "ndjson" # or text

Signed-off-by: Luke Steensen <luke.steensen@gmail.com>
Signed-off-by: Luke Steensen <luke.steensen@gmail.com>
Signed-off-by: Luke Steensen <luke.steensen@gmail.com>
Signed-off-by: Luke Steensen <luke.steensen@gmail.com>
@lukesteensen lukesteensen added type: bug A code related bug. sink: console Anything `console` sink related domain: config Anything related to configuring Vector sink: file Anything `file` sink related labels Oct 15, 2019
Signed-off-by: Luke Steensen <luke.steensen@gmail.com>
Signed-off-by: Luke Steensen <luke.steensen@gmail.com>
#[serde(default)]
pub target: Target,
pub encoding: Option<Encoding>,
pub encoding: Encoding,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@LucioFranco shouldn't this have been resolved in #894? I assume we just missed this? Making sure this wasn't intentional.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe I discussed this with you or luke but the reason this sink allows a null encoding is that it makes sense here to auto change how logs are printed since this is a debug sink. Happy to go the other way but for me this one actually made sense to dynamically choose since the downstream service is a human instead of a machine that might expect an encoding.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't remember having that conversation. Do you have this documented anywhere? It should be in Github or Slack somewhere, correct?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'd have been ok with leaving it, but I only noticed this because the logic was complex enough that we skipped an entire case and weren't outputting anything at all. That convinced me we should simplify.

Ok(converted) => log.insert_explicit(field.into(), converted),
Err(error) => {
debug!(
warn!(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 I believe this transform was merged before we had rate limiting. Unfortunately, #628 didn't cover this transform. I'll do another pass to make sure we're not missing anything new.

Copy link
Copy Markdown
Contributor

@binarylogic binarylogic left a comment

Choose a reason for hiding this comment

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

This all looks great! Thanks for fixing.

Signed-off-by: Ben Johnson <bjohnson@binarylogic.com>
@lukesteensen
Copy link
Copy Markdown
Member Author

@binarylogic do you know why the semantic PR check is still pending? I thought I did it right...

@binarylogic binarylogic changed the title fix(config)!: require encoding for console and file sinks fix(config): require encoding for console and file sinks Oct 16, 2019
@binarylogic binarylogic changed the title fix(config): require encoding for console and file sinks fix(config)!: require encoding for console and file sinks Oct 16, 2019
@binarylogic
Copy link
Copy Markdown
Contributor

It's the !. I opened zeke/semantic-pull-requests#73.

Copy link
Copy Markdown
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.

I added a comment about encoding in console but I'm happy either way.

@binarylogic
Copy link
Copy Markdown
Contributor

Anything holding this up from merging? I would merge regardless of the semantic check for now.

@lukesteensen lukesteensen merged commit 616d14a into master Oct 21, 2019
@lukesteensen lukesteensen deleted the guide-issues branch October 21, 2019 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain: config Anything related to configuring Vector sink: console Anything `console` sink related sink: file Anything `file` sink related type: bug A code related bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Help] coercer transform

3 participants