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

[Filebeat] - Add raw json field when unmarshaling fails #6591

Merged

Conversation

crazyvikas23
Copy link
Contributor

@crazyvikas23 crazyvikas23 commented Mar 19, 2018

Fixes #6516.

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@ruflin
Copy link
Contributor

ruflin commented Mar 19, 2018

jenkins, test it

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

I would like to see the raw JSON contained in the message field on error. Would that be possible to implement?

@crazyvikas23
Copy link
Contributor Author

crazyvikas23 commented Mar 20, 2018

@andrewkroh I think you are right, message key inside error should only contain error message. I have added raw json data in message field outside.

if jsonFields == nil {
jsonFields = common.MapStr{}
}
jsonFields["message"] = fmt.Sprintf("Raw json data: %s", string(text))
Copy link
Member

@andrewkroh andrewkroh Mar 20, 2018

Choose a reason for hiding this comment

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

I think this is going to produce json.message in the final event and I'd like to have just message. It looks to me like the message.Content field is being populated with the text (raw json) when an error occurs.

So I think something upstream in the harvester needs to change to allow the fields["message"] = message.Content to be set. I think the issue lies within this code:

text := string(message.Content)
// Check if data should be added to event. Only export non empty events.
if !message.IsEmpty() && h.shouldExportLine(text) {
fields := common.MapStr{
"source": state.Source,
"offset": state.Offset, // Offset here is the offset before the starting char.
}
fields.DeepUpdate(message.Fields)
// Check if json fields exist
var jsonFields common.MapStr
if f, ok := fields["json"]; ok {
jsonFields = f.(common.MapStr)
}
data.Event = beat.Event{
Timestamp: message.Ts,
}
if h.config.JSON != nil && len(jsonFields) > 0 {
ts := reader.MergeJSONFields(fields, jsonFields, &text, *h.config.JSON)
if !ts.IsZero() {
// there was a `@timestamp` key in the event, so overwrite
// the resulting timestamp
data.Event.Timestamp = ts
}
} else if &text != nil {
if fields == nil {
fields = common.MapStr{}
}
fields["message"] = text
}
data.Event.Fields = fields
}

@ruflin I think we need your Filebeat expertise on this modification.

Copy link
Contributor Author

@crazyvikas23 crazyvikas23 Mar 21, 2018

Choose a reason for hiding this comment

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

I think what you want is to append a message key in the final event. That should be pretty straightforward to implement. I have done that change but it seems too hackish.

@crazyvikas23 crazyvikas23 force-pushed the crazyvikas23/add_raw_json_data_filebeat branch from 7f084c2 to 496ad20 Compare March 21, 2018 07:37
@crazyvikas23 crazyvikas23 force-pushed the crazyvikas23/add_raw_json_data_filebeat branch from 496ad20 to eedc6f3 Compare March 22, 2018 07:10
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

LGTM. It would be good to have a test case added to json_test.go that exercises the new code you added.

@@ -30,7 +30,8 @@ func (r *JSON) decodeJSON(text []byte) ([]byte, common.MapStr) {
err := unmarshal(text, &jsonFields)
if err != nil || jsonFields == nil {
if !r.cfg.IgnoreDecodingError {
logp.Err("Error decoding JSON: %v", err)

logp.Err("Error decoding JSON: %v. Raw data: %s", err, string(text))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we shouldn't put this raw message into the error logs. I'm worried about sensitive information being leaked inside of the Filebeat logs.

// handle the case in which r.cfg.AddErrorKey is set and len(jsonFields) == 1
// and only thing it contains is `error` key due to error in json decoding
// which results in loss of message key in the main beat event
if len(jsonFields) == 1 && jsonFields["error"] != nil {
Copy link
Member

Choose a reason for hiding this comment

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

That looks like it accomplishes it. 👍

@crazyvikas23
Copy link
Contributor Author

@andrewkroh I don't think we can write a test case for same in json_test.go because file it contains unit test cases for json decoding, which already works fine. We should take this as integration test elsewhere.

@crazyvikas23 crazyvikas23 force-pushed the crazyvikas23/add_raw_json_data_filebeat branch from ca9253d to 2372894 Compare April 13, 2018 11:40
@andrewkroh
Copy link
Member

jenkins, test it

@andrewkroh andrewkroh changed the title add raw json field when json unmarshaling fails in filebeat [Filebeat] - Add raw json field when unmarshaling fails Apr 13, 2018
@andrewkroh
Copy link
Member

jenkins, test it

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

Successfully merging this pull request may close these issues.

[Filebeat] Data loss when JSON decoding fails
4 participants