search

openzipkin / zipkin-js

Zipkin instrumentation for Node.js and browsers
Apache License 2.0
564 stars 171 forks source link

isOptional and Related checks are wrong and breaks compatibility between versions #518

Open whizzzkid opened 4 years ago

whizzzkid commented 4 years ago

Namaste Team

The isOptional check seems to breaking our version compatibility. Our projects use v 0.19 and 0.22 and we have been trying to figure why is our tracing broken using hapi. We seem to have narrowed it down to the bad isOptional check here: https://github.com/openzipkin/zipkin-js/blob/master/packages/zipkin/src/option.js#L65

Log from our service looks like so:

(node:19) UnhandledPromiseRejectionWarning: Error: Error: data (Some(true)) is not an Option!
    at verifyIsOptional (/app/node_modules/service-container/node_modules/zipkin/lib/index.js:223:11)
    at new TraceId (/app/node_modules/service-container/node_modules/zipkin/lib/index.js:773:5)
    at Tracer.createChildId (/app/node_modules/service-container/node_modules/zipkin/lib/index.js:1130:21)
    at HttpClientInstrumentation.recordRequest (/app/node_modules/service-container/node_modules/zipkin/lib/index.js:1929:37)
    at /app/node_modules/service-container/lib/requestRouter/wrapWreckRequest.js:32:48
    at ExplicitContext.scoped (/app/node_modules/service-container/node_modules/zipkin/lib/index.js:1347:16)
    at Tracer.scoped (/app/node_modules/service-container/node_modules/zipkin/lib/index.js:1090:28)
    at /app/node_modules/service-container/lib/requestRouter/wrapWreckRequest.js:31:75
    at new Promise (<anonymous>)

Where service-container is a dependency that bootstraps our service. The app level version is using zipkin@0.22 and the dependecy is using zipkin@0.19. This breaks how we generate trace instance and can be seen in this Proof of Concept PR: https://github.com/openzipkin/zipkin-js/pull/517/files

This, essentially means we cannot even upgrade to fix the issue, because the instanceof check does not seem to be the right way to check for instances. In the POC, even though the class signature is same, isOptional fails.

Please Advise!

eirslett commented 4 years ago

Have you tried using zipkin as a peerDependency instead of a regular dependency?

whizzzkid commented 4 years ago

We have not @eirslett, is that in the pitfalls/faqs which we missed?

eirslett commented 4 years ago

I don't think it's written anywhere. I remember vaguely that this was the solution to a similar problem I encountered some years ago. Using peerDependency will ensure that all your modules are using the same zipkin dependency. What you should do is change the package.json of your service-container source code - and make zipkin a peerDependency. Your main application should have zipkin as a "normal" dependency. Look inside the node_modules folder after running npm install, just to verify that's the case. (Also, have a look at package-lock.json)

(I'm sorry about the Optional/Some/None thing, it was a terrible design mistake anyway. The code was ported from Scala/Twitter, too literally. Should have used nullable types in JS instead.)

whizzzkid commented 4 years ago

Ok, thanks @eirslett will give this a shot, really appreciate your quick turnaround.

jcchavezs commented 4 years ago

Please let us know if that fixes the issue for you, hapi is breaking on async await. Check this issue about it: https://github.com/openzipkin/zipkin-js/issues/483#issuecomment-583820036