Skip to content

Add JSON options to autodiscover hints#14208

Merged
ChrsMark merged 6 commits intoelastic:masterfrom
ChrsMark:add_json_hints
Oct 25, 2019
Merged

Add JSON options to autodiscover hints#14208
ChrsMark merged 6 commits intoelastic:masterfrom
ChrsMark:add_json_hints

Conversation

@ChrsMark
Copy link
Copy Markdown
Member

@ChrsMark ChrsMark commented Oct 23, 2019 •
edited
Loading

Closes #12634.

cc: @odacremolbap, @exekias

Manual testing

Configure filebeat autodiscover

filebeat:
  autodiscover.providers:
    - type: docker
      hints.enabled: true

Having filebeat running, launch containers with the proper labels each time.
Make sure that Filebeat has permissions to read from /var/lib/docker/containers/, if not a quick option could be to run with sudo like sudo -E ./filebeat -e -d "*" --strict.perms=false. Note that for testing this a Linux host is needed so as Filebeat to be able to locate and read log files properly.

Test that keys are not copied under top level in the output document

docker run -l co.elastic.logs/json.keys_under_root=false -it busybox echo '{"foo":"bar", "error_msg": "error opening file"}'

You should see in the event the following:

"json": {
      "foo": "bar",
      "error_msg": "error opening file"
    },

Test keys are copied under top level in the output document

docker run -l co.elastic.logs/json.keys_under_root=true -it busybox echo '{"foo":"bar", "error_msg": "error opening file"}'

You should see the foo and error_msg in top level.

Test that error is reported

docker run -l co.elastic.logs/json.keys_under_root=true -l co.elastic.logs/json.add_error_key=true -it busybox echo '{"foo":"bar", "error_msg": "error opening file" and some buggy stuff}'

Event should be enriched with:

"error": {
      "type": "json",
      "message": "Error decoding JSON: invalid character 's' after object key:value pair"
    },

@ChrsMark ChrsMark added enhancement containers Related to containers use case Team:Integrations Label for the Integrations team autodiscovery test-plan Add this PR to be manual test plan v7.6.0 labels Oct 23, 2019
@ChrsMark ChrsMark requested a review from a team as a code owner October 23, 2019 11:01
@ChrsMark ChrsMark self-assigned this Oct 23, 2019
houndci-bot
houndci-bot reviewed Oct 23, 2019
View reviewed changes
@ChrsMark
Add JSON options to hints
084ccc5 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark
fix linter
c87e5bb 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark changed the title Add JSON options to hints Add JSON options to autodiscover hints Oct 23, 2019
@ChrsMark
Add changelog and docs
e47671a 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark
Add tests
b0a6b7f 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@jsoriano
Copy link
Copy Markdown
Member

jsoriano commented Oct 24, 2019

Great addition!

exekias
exekias approved these changes Oct 24, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

Looking good!

exekias
exekias suggested changes Oct 24, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@exekias exekias left a comment •
edited
Loading

Choose a reason for hiding this comment

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

sorry didn't notice failing tests, this will require a make update, and probably a rebase

@exekias
Copy link
Copy Markdown
Contributor

exekias commented Oct 24, 2019

Also, outside of the scope of this PR (maybe a followup). What would you think about simplifying these parameters for the user? I'm thinking something like this would make things better:

co.elastic.logs/json.enabled=true

This somehow would be an alias for:

co.elastic.logs/json.message_key=message

which is probably what most users want

@ChrsMark
fmt
2bcc6a4 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark
Copy link
Copy Markdown
Member Author

ChrsMark commented Oct 24, 2019

Also, outside of the scope of this PR (maybe a followup). What would you think about simplifying these parameters for the user? I'm thinking something like this would make things better:

co.elastic.logs/json.enabled=true

This somehow would be an alias for:

co.elastic.logs/json.message_key=message

which is probably what most users want

🤔 could you elaborate more?

@exekias
Copy link
Copy Markdown
Contributor

exekias commented Oct 25, 2019 •
edited
Loading

Sure! sorry for my short explanation 🤕

I think that current config keys are not very straight forward. Having that in most cases what users want is probably to just decode the message field as a JSON, I think we should add a more intuitive setting name, that will just do that. This could be co.elastic.logs/json.enabled=true, which should enable JSON decoding from the message field.

This would be the same behavior as using co.elastic.logs/json.message_key=message today

@exekias
Copy link
Copy Markdown
Contributor

exekias commented Oct 25, 2019

@ChrsMark had a chat and we decided it probably makes more sense to have the enabled keyword in the original JSON settings if possible, I will create an issue for that

@ChrsMark
Copy link
Copy Markdown
Member Author

ChrsMark commented Oct 25, 2019

jenkins, test this again please

@ChrsMark
Copy link
Copy Markdown
Member Author

ChrsMark commented Oct 25, 2019

Merging this since Travis went green and failing tests of Jenkins look irrelevant to this patch.

@ChrsMark ChrsMark merged commit a5d5f91 into elastic:master Oct 25, 2019
@ChrsMark ChrsMark mentioned this pull request Dec 16, 2019
Closed
@ycombinator ycombinator self-assigned this Jan 9, 2020
@ycombinator
Copy link
Copy Markdown
Contributor

ycombinator commented Jan 9, 2020

Assigning myself for manually testing this PR for 7.6.0 release per test-plan label.

@ycombinator
Copy link
Copy Markdown
Contributor

ycombinator commented Jan 14, 2020

Tested manually. Works as expected!

@ycombinator ycombinator added test-plan-ok This PR passed manual testing and removed test-plan-ok This PR passed manual testing labels Jan 14, 2020
@ycombinator
Copy link
Copy Markdown
Contributor

ycombinator commented Jan 15, 2020

Removed test-plan-ok label for now. Will re-test with BC (previously I tested by building from the 7.x branch).

@ycombinator
Copy link
Copy Markdown
Contributor

ycombinator commented Jan 16, 2020 •
edited
Loading

Tested with BC1 and this works as expected!

@ycombinator ycombinator added the test-plan-ok This PR passed manual testing label Jan 16, 2020
jorgemarey pushed a commit to jorgemarey/beats that referenced this pull request Jun 8, 2020
@ChrsMark @jorgemarey
Add JSON options to autodiscover hints (elastic#14208)
f61bf0d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@exekias exekias exekias approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ycombinator ycombinator

Labels

autodiscovery containers Related to containers use case enhancement review Team:Integrations Label for the Integrations team test-plan Add this PR to be manual test plan test-plan-ok This PR passed manual testing v7.6.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add JSON options to hints

6 participants

@ChrsMark @jsoriano @exekias @ycombinator @houndci-bot @andresrc