lightstep / lightstep-tracer-javascript

Lightstep distributed tracing library for Node.js and the browser
https://lightstep.com
MIT License
77 stars 66 forks source link

Span startTime should handle decimal values for nanoseconds #145

Closed sbaum1994 closed 5 years ago

sbaum1994 commented 5 years ago

Opentracing docs state that decimals for nanoseconds in start times on spans is supported: https://opentracing-javascript.surge.sh/interfaces/spanoptions.html

However we only account for microseconds and not nanoseconds in our handling of startTime see below: https://github.com/lightstep/lightstep-tracer-javascript/blob/744a2978c457065e85ce46f7b3961276477970be/src/imp/tracer_imp.js#L275

Our satellites expect integers, resulting in an Go unmarshalling error on the satellite side when nanosecond decimals are passed in to startTime using this tracer

-- Issue brought to attention by @Rowno please add more details if I missed anything or described it incorrectly!

Rowno commented 5 years ago

This also applies to Span.finish().

austinlparker commented 5 years ago

Thanks for opening the issue, we should be able to address this shortly.

austinlparker commented 5 years ago

@Rowno I've been looking into this, and we don't actually support nanosecond resolution when we ingest spans. I can make a PR that fixes the problem on the client side, but it'll just be ceil/floor on the microsecond value there.

Rowno commented 5 years ago

Yeah, that's fine. It's just confusing when you point at the opentracing docs but don't fully support the spec 😅

austinlparker commented 5 years ago

Yeah, word, that's fair. We'll get it in for the next release.