-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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 HTML escaping during JSON Marshaling #4379
Conversation
By default the golang JSON Marshaller escapes HTML entries. As this is not required for beats this is disabling this option. * Unification of json marshalling Closes elastic#2581
@@ -67,7 +67,6 @@ func (rotator *FileRotator) WriteLine(line []byte) error { | |||
} | |||
} | |||
|
|||
line = append(line, '\n') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not 100% clear to me what this change was needed. Seems like before there was no newline at the end of each event and now there is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick test comparing json.Marshal() to common.JSONEncode() would be interesting to see if they produce the same output or if one has a newline.
Is this code also used by the logp to write files? Does this affect it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I would like to have a better idea about that newline change before merging.
import ( | ||
"bytes" | ||
"encoding/json" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove newline.
enc := json.NewEncoder(buffer) | ||
enc.SetEscapeHTML(true) | ||
|
||
enc.SetIndent("", "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the default which would make this line unnecessary?
|
||
err := enc.Encode(data) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to convert the data to JSON (%v): %#v", err, data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use errors.Wrapf here.
@@ -67,7 +67,6 @@ func (rotator *FileRotator) WriteLine(line []byte) error { | |||
} | |||
} | |||
|
|||
line = append(line, '\n') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick test comparing json.Marshal() to common.JSONEncode() would be interesting to see if they produce the same output or if one has a newline.
Is this code also used by the logp to write files? Does this affect it?
@andrewkroh I also don't think we should merge this without having figured out where the newline change is coming from. Unfortunately will probably not have time for it this week. |
I'm going to close this PR as I never found the time to further investigate and the code base has changed quite a bit since then so a restart makes more sense. @urso I think disabling the escaping would still be the way to go. Also having one method to Marshal json would be nice in libbeat so potential optimisations could be made in a central place. I leave it up to you if you want to pursue this further. |
By default the golang JSON Marshaller escapes HTML entries. As this is not required for beats this is disabling this option.
Closes #2581