Skip to content

Add merge-json-log to event router#17040

Merged
mburke5678 merged 6 commits intoopenshift:masterfrom
mburke5678:logging-event-router-merge-jspn
Oct 25, 2019
Merged

Add merge-json-log to event router#17040
mburke5678 merged 6 commits intoopenshift:masterfrom
mburke5678:logging-event-router-merge-jspn

Conversation

@mburke5678
Copy link
Contributor

@mburke5678 mburke5678 commented Oct 3, 2019

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 3, 2019
@openshift-docs-preview-bot

The preview will be available shortly at:

@mburke5678 mburke5678 added this to the Next Release milestone Oct 3, 2019
@mburke5678 mburke5678 added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 3, 2019
@mburke5678
Copy link
Contributor Author

Hold PR pending possible code change per @richm

@mburke5678 mburke5678 changed the title Add merge-json-log to event router [WIP] Add merge-json-log to event router Oct 3, 2019
Copy link

@anpingli anpingli left a comment

Choose a reason for hiding this comment

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

LGTM

@richm
Copy link

richm commented Oct 8, 2019

nack - so the PR openshift/origin-aggregated-logging#1766 will basically enable merge_json_log implicitly for eventrouter events (assuming the documentation has been followed to deploy the eventrouter) - with that PR, the user does not have to enable MERGE_JSON_LOG=true to use eventrouter

@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 8, 2019
Copy link

Choose a reason for hiding this comment

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

Fluentd will automatically parse JSON logs that come from Pods whose names match eventrouter-*, which is the naming convention used in the example configuration.

Copy link

Choose a reason for hiding this comment

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

That is, I don't think we should mention MERGE_JSON_LOG here.

Copy link

@richm richm Oct 8, 2019

Choose a reason for hiding this comment

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

Instead of this, say something like

You must set `TRANSFORM_EVENTS=true` in order to process and store eventrouter events in Elasticsearch.  In order to do this, you must set cluster logging to the unmanaged state.

Then add a link to the docs where we explain how to set cluster logging to the unmanaged state. Then

oc set env ds/fluentd TRANSFORM_EVENTS=true

@richm
Copy link

richm commented Oct 8, 2019

should I close this PR?

It depends - if we have already documented that you have to set TRANSFORM_EVENTS=true and set cluster logging to unmanaged in order to use eventrouter, then yes, you can close this PR.

@mburke5678
Copy link
Contributor Author

@richm

should I close this PR?

Sorry, I didn't see your comment on TRANSFORM_EVENTS until after I posted that comment.
I added TRANSFORM_EVENTS in the latest commit

@mburke5678 mburke5678 force-pushed the logging-event-router-merge-jspn branch from 633a3ac to f56c4f2 Compare October 8, 2019 17:05
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 8, 2019
Copy link

@richm richm left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 8, 2019
@mburke5678
Copy link
Contributor Author

@anpingli Can I ask you to take another look based on Rich's comments?

@mburke5678 mburke5678 merged commit 69ca7fb into openshift:master Oct 25, 2019
@mburke5678 mburke5678 changed the title [WIP] Add merge-json-log to event router Add merge-json-log to event router Oct 25, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 25, 2019
@mburke5678 mburke5678 deleted the logging-event-router-merge-jspn branch January 22, 2020 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.1 branch/enterprise-4.2 lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants