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

Cast span_id and trace_id as string when adding to the record. #1547

Merged
merged 3 commits into from
Aug 28, 2020

Conversation

arthurio
Copy link
Contributor

If you are using a json logger the record value will be interpreted as a number which will result in values being rounded in the log pipeline.

@arthurio arthurio requested a review from a team as a code owner June 30, 2020 21:19
@majorgreys majorgreys self-assigned this Aug 14, 2020
@majorgreys
Copy link
Collaborator

@arthurio This fix makes good sense and we can certainly use strings for the record attributes. Do you mind sharing an example log entry where this rounding happened? I'm wondering if its a case of an int being truncated rather than rounded.

@arthurio
Copy link
Contributor Author

This fix makes good sense and we can certainly use strings for the record attributes. Do you mind sharing an example log entry where this rounding happened? I'm wondering if its a case of an int being truncated rather than rounded.

@majorgreys Thanks for getting back to me, this is the screenshot I took when reporting the bug:

Live_Tail___Datadog_and_Forum___Minerva_Project

FYI we use python-json-logger.

Kyle-Verhoog
Kyle-Verhoog previously approved these changes Aug 28, 2020
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

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

Thanks @arthurio! It won't let me merge in master so let us know once it's back up to date and we can get it in 😄

@jd
Copy link
Contributor

jd commented Aug 28, 2020

It'd be great to have a test so we don't break it in the future.

arthurio and others added 2 commits August 28, 2020 18:09
If you are using a json logger the record value will be interpreted
as a number which will result in values being rounded in the log
pipeline.
Co-authored-by: Tahir H. Butt <tahir@tahirbutt.com>
@arthurio arthurio force-pushed the log-as-string branch 2 times, most recently from 2fe0521 to f135166 Compare August 28, 2020 20:19
Kyle-Verhoog
Kyle-Verhoog previously approved these changes Aug 28, 2020
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

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

Thanks for the test and quick response @arthurio!! 😄

@arthurio
Copy link
Contributor Author

@Kyle-Verhoog No problem, thanks for your help. And are those assertions enough testing?

@Kyle-Verhoog
Copy link
Member

Yeah I think it'd be overkill to do anything further. Future people can reference this PR for the justification.

@Kyle-Verhoog
Copy link
Member

@arthurio
Copy link
Contributor Author

@Kyle-Verhoog Ok, I switched to a filter class to accommodate for both.

Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

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

🙌

@Kyle-Verhoog Kyle-Verhoog merged commit 32dcfc9 into DataDog:master Aug 28, 2020
@arthurio arthurio deleted the log-as-string branch August 28, 2020 21:57
@Kyle-Verhoog Kyle-Verhoog added this to the 0.42.0 milestone Aug 28, 2020
@majorgreys majorgreys modified the milestones: 0.42.0, 0.43.0 Sep 10, 2020
@majorgreys majorgreys modified the milestones: 0.43.0, 0.42.0 Sep 10, 2020
majorgreys pushed a commit that referenced this pull request Sep 10, 2020
…record. (#1547)

If you are using a json logger the record value will be interpreted
as a number which will result in values being rounded in the log
pipeline.

* Cast span_id and trace_id as string when adding to the record.

* Test that span_id and trace_id are strings in the log record.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants