Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Converting timestamp and duration to the number type to follow Zipkin span types #231

Conversation

dsborets
Copy link
Contributor

In order to follow a Zipkin Span specification, I'm proposing to change string type to number for timestamp and duration.

@justindsmith
Copy link
Contributor

Thanks @dsborets!

Just to be sure, did you validate this change against a zipkin endpoint to make sure the data is received and handled correctly in zipkin? Just want to make sure there are no breaking change, otherwise looks good.

@mayurkale22
Copy link
Member

LGTM, please address @justindsmith's comment.

@dsborets
Copy link
Contributor Author

Hi @justindsmith . Sure, I tested it and found that Zipkin accepting any spans disregards of timestamp and duration fields type (string or number). So we should be safe. I found this issue when we started to use Zipkin-to-Datadog proxy. And that proxy following Zipkin specification accepting only number type (simply by unmarshalling json to Go structure). Initially I thought it's a proxy's problem, but finally realized type discrepancy in opencensus exporter. Probably nobody used opencensus with any servers with more strict type casting than standard Zipkin server

Copy link
Contributor

@justindsmith justindsmith 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 clarification @dsborets. Looks good to me!

@justindsmith justindsmith merged commit e4e19ea into census-instrumentation:master Dec 11, 2018
@dsborets
Copy link
Contributor Author

Thank you 👍

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

Successfully merging this pull request may close these issues.

None yet

4 participants