-
Notifications
You must be signed in to change notification settings - Fork 12
Fix span detail format #79
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
Conversation
Format spans in format expected by Test Engine.
Align type with Test Engine, simplify implementation. Make pylint happy
d286f98 to
0ea0fa4
Compare
nprizal
left a comment
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.
Hi @scadu, thanks for opening the PR. It looks great! I appreciate you adding the codedoc around the expected data structure for span details.
We’re planning to implement validation to prevent the collector from sending invalid span data but we’ll open a separate PR for that.
nprizal
left a comment
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.
Thanks for adding the validation 💚 . I think we want the detail to be required for sql, http, and annotation. Is that right @gchan @malclocke?
Yep that's correct. Here's the reference which should be up-to-date and correct. |
Co-authored-by: Naufan P. Rizal <np.rizal@icloud.com>
nprizal
left a comment
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.
🚀
|
Thanks for the review @nprizal! I think it should be good to merge now? |
Format spans in format expected by Test Engine.
Since
detailstype is changed fromstringtodict, this could be a breaking change, even though span details previously wouldn't work due to the type mismatch between collector and Test Engine.