-
Notifications
You must be signed in to change notification settings - Fork 437
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
ddtracer/tracer: marshalled span values in encode span #832
Conversation
Illegal characters, such as newlines, were not escaped in encodeSpan function, which ruined json structure. Closes #791
Added same marshalling to encoding metrics keys. Also, added error logging to both metrics and meta marshalling
I'm interested in this fix since it's affecting me too: I appreciate that it's being worked on! Should this fix be widened such that the same technique is used in all cases where The spec has details of the cases to cover, and the Lastly, should this use an explicit |
@jamestelfer you're right. We should use this everywhere where we write strings that are user input and aren't numeric (which get converted easily), such as It'll likely help to create a helper function |
@gbbr / @dianashevchenko LMK if you'd like me to pitch in here! I'm currently blocked on this and would be happy to. |
@jamestelfer all comments welcome and help is much appreciated! |
Service, resource, and operation names were too marshalled to make sure they don't break JSON with invalid characters such as newlines
This is looking great, thanks @dianashevchenko! I'm going to pull this branch and test it against the service I have that's failing. |
I forked this branch and merged Looks good to me, and thanks again! |
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!
@jamestelfer thanks for confirming that this fixes your issue. |
Illegal characters, such as newlines, were not escaped in encodeSpan function, which ruined json structure. Closes #791
This, in turn, corrupted log collection