Skip to content

Conversation

@bendrucker
Copy link
Contributor

@bendrucker bendrucker commented Jan 15, 2019

  • Adds timestamp_key property, which allows the user to customize the default key of timestamp (+ tests)
  • Changes merge order of json_merge so that the log message wins over the record data in cases of conflict (+ tests)
  • Updates bundler to 2 (unrelated, required to run tests)

Notes

  • Switching the symbol key ({ :timestamp => 123 }) to a string { 'timestamp' => 123 } is important. JSON.dump({ :foo => 'bar', 'foo' => 'qux' }) will return {"foo:"bar","foo":"qux"} so we need key types to match.
  • The change in merge order is the most invasive but it seems sensible to prioritize user data over record data in the event of conflict. I doubt anyone has run into this yet and I had to write a failing test to understand how it works.

Motivation

Scoop uses a standard logging library across our apps that emits newline-delimited JSON. It includes timestamps as {"time": 123}. A dev noticed that log lines from Kubernetes contained both timestamp and time. Because we use json_merge, both properties appear at the same level. We'd like to configure fluentd to render a timestamp as time to any log line missing it (including non-JSON) but not overwrite an existing time property in JSON lines.

@bendrucker bendrucker changed the title config: add "timestamp_key" for customizing "timestamp" Add "timestamp_key", prefer log message when merging Jan 16, 2019
@frankreno
Copy link
Contributor

frankreno commented Jan 17, 2019

LGTM, thanks! I'll be cutting a new release tomorrow or Friday. Will you be submitting a PR to the Helm repo once this gets released?

edit: HAH, Im mixing my repos! I am going to cut a new FluentD K8S image, when I do, ill upgrade this plugin version to ruby gems and update it in the k8s plugin

@frankreno frankreno merged commit 48295f7 into SumoLogic:master Jan 17, 2019
@bendrucker
Copy link
Contributor Author

I'll send a PR to the k8s repo adding the ability to configure this via env and then add it to the chart as well

@frankreno
Copy link
Contributor

@bendrucker no worries on that, already started as I also want to upgrade the fluentD image...working on that but hit a couple snags I am working thru

@bendrucker
Copy link
Contributor Author

Sounds good!

@bendrucker bendrucker deleted the timestamp-key branch January 17, 2019 16:44
@frankreno
Copy link
Contributor

@bendrucker - this is now available in the k8s fluentD plugin.

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.

2 participants