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

Disable merge JSON log in 4.0 #1492

Closed
jcantrill opened this issue Jan 3, 2019 · 7 comments · Fixed by #1493
Closed

Disable merge JSON log in 4.0 #1492

jcantrill opened this issue Jan 3, 2019 · 7 comments · Fixed by #1493
Assignees

Comments

@jcantrill
Copy link
Contributor

Issue

Reviewing logs from a production cluster led to the creation of [1]. During discussion in the issue and slack, it its my understanding the root cause is:

  • messages that are JSON logged are merged into the fluent payload by default
  • Elasticsearch dynamic mappings cause a new mapping to be generated when a field is first indexed
  • Subsequent indexing will fail if the value in the later documents does not match the original (e.g. int vs. string)

This subsequently is leading to message loss as identified in [1] since the log entry is rejected by ES (Does it cycle indefinitely?).

Proposal

This proposal would disable the merge JSON log feature by default in the 4.0 delivery and would:

  • Make it difficult to revert (operator=unmanaged, set env var?)
  • Document deprecation, ramifications if enabled
  • Introduce a JIRA card to re-enable in an alternate capacity (e.g. whitelist, project logging CR, subfield)

[1] Ref: #1491

@richm
Copy link
Contributor

richm commented Jan 3, 2019

NACK - many customers use and depend on this feature

@nhosoi
Copy link
Contributor

nhosoi commented Jan 3, 2019

It may not affect us, but merge_json_log is discontinued from fluent-plugin-kubernetes_metadata_filter:
NOTE: As of the release 2.1.x of this plugin, it no longer supports parsing the source message into JSON and attaching it to the payload. The following configuration options are removed:

  • merge_json_log
  • preserve_json_log

@jcantrill
Copy link
Contributor Author

jcantrill commented Jan 3, 2019

@richm We should consider this in context of the proposed release, 4.0. The release is green-field only deployments. In the current incarnation, this feature is broken:

  • It stomps existing fields
  • It shows we have data loss, possibly a memory leak of sorts where records cycle back to retry queues
  • It does not properly handle the case where 2 containers create log messages with the same fields but of different types

This proposal does not advocate removing it, but disabling it by default and adding documentation to clearly define the drawbacks. Until we can devise a proper solution, customer's should understand what they are getting and why we have disabled it.

@richm
Copy link
Contributor

richm commented Jan 3, 2019

It may not affect us, but merge_json_log is discontinued from fluent-plugin-kubernetes_metadata_filter:
NOTE: As of the release 2.1.x of this plugin, it no longer supports parsing the source message into JSON and attaching it to the payload. The following configuration options are removed:

* `merge_json_log`

* `preserve_json_log`

Right - it doesn't affect us (it is us who removed that feature from fluent-plugin-kubernetes_metadata_filter . . . because of this problem) - we have our own implementation - https://github.com/openshift/origin-aggregated-logging/blob/master/fluentd/configs.d/filter-parse-json-field.conf

@richm
Copy link
Contributor

richm commented Jan 3, 2019

ok - so we disable it by default, and only for 4.0 - to your list of tasks I would add

  • change the json-parsing test to enable the feature before testing (by setting cluster to unmanaged, etc.)
  • come up with a real solution to the problem in 4.1

@jcantrill
Copy link
Contributor Author

@jcantrill
Copy link
Contributor Author

Partially addressed in openshift/cluster-logging-operator#74

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants