search

openzipkin / zipkin-js

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

Question: add instrumentation support of koa (HTTP framework) #112

Closed shumsky closed 5 years ago

shumsky commented 7 years ago

Hi,

Recently I needed to integrate a koa based microservice with zipkin. I didn't find any existing integrations and had to write my custom middleware. Now I'm thinking about contributing it to open source. Would it be reasonable to add corresponding module to zipkin-js/packages?

codefromthecrypt commented 7 years ago

@vicanso I've seen you have some experience with koa and zipkin. Do you have any middleware put together like this?

vicanso commented 7 years ago

@adriancole I am not familiar with zipkin and I am not going to write the middleware at this moment. Sorry for any inconvenience.

kohv commented 7 years ago

@shumsky I would be interested. Maybe you could create a PR for this?

shumsky commented 7 years ago

@kohv Yes, I'm going to work on it.

shumsky commented 7 years ago

Hi,

I'm ready to open a PR. But I've just realized there might be a problem. I've written a middleware for Koa 2.x which requires Node 7 and higher (because of async/await). There are two points.

  1. zipkin-js is transpiled to ES2015. This doesn't seem to be a problem, since async-await is transpiled into promises and generators which works fine with Koa 2.x.

  2. Travis runs tests on Node 4, 6 and 8. This is an issue. I guess it is not possible to exclude 4 and 6 for my middleware package.

@adriancole maybe you could advise here?

davsucks commented 5 years ago

@shumsky Any reason you closed your own PR? I'm in a similar position (needing to orchestrate a koa 2.x.x application) and I'd love to avoid duplicating your effort 😄

shumsky commented 5 years ago

Hey @davsucks. I closed it because Travis build was failing and I couldn't figure out how to fix it. I described the problem in my previous comment. Travis build was running tests on different versions of Node including 4 and 6 which are not supported by Koa 2. That was almost 2 years ago and something might have changed since then. I'll try to check this out today/tomorrow.

jcchavezs commented 5 years ago

Awesome!

codefromthecrypt commented 5 years ago

ps drift alert.. I plan to rewrite the server tests tomorrow similar to the http client ones (https://github.com/openzipkin/zipkin-js/pull/427). It should make implementing this easier.

jcchavezs commented 5 years ago

@shumsky currently this library is being tested for node8+ and we got much more solid tests so worth to give it a try.

codefromthecrypt commented 5 years ago

drift alert is mostly contained, and I'll take responsibility for merge work if later drift affects you

shumsky commented 5 years ago

Ok, I've started working on it. Rebased my branch onto the upstream, made some alignments in my code, and then discovered that one of the integration tests is failing. I haven't been working with Zipkin for quite a while, so I guess it may take some time to refresh my knowledge and fix it, but I'm on it.

@adriancole thanks for notifying.

jcchavezs commented 5 years ago

@shumsky feel free to reach for help in https://gitter.im/openzipkin/zipkin

codefromthecrypt commented 5 years ago

yep if it is in "basic tests" and it is about the post test hook (which checks for leaks) note that the http server instrumentation functions leak by default. you need to use scoped to stop that. have a look at some of the other instrumentation about how that's fixed.

meanwhile we hope to have a non-leaky api eventually

On Mon, Jul 29, 2019 at 10:52 PM José Carlos Chávez notifications@github.com wrote:

@shumsky feel free to reach for help in https://gitter.im/openzipkin/zipkin

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

shumsky commented 5 years ago

The PR is ready I think. Though I discovered that there's already a package in npm with the same name: https://www.npmjs.com/package/zipkin-instrumentation-koa I didn't review the existing package in detail. Maybe there's no sense in publishing the module I've written. Or if there is, then it has to be renamed at least.

codefromthecrypt commented 5 years ago

@agreatfool are you ok with us obviating your package? It might be easier on you to have help maintaining this. Otherwise we need to choose a mildly different name like we have done in the past. ex zipkin-instrumentation-koajs

agreatfool commented 5 years ago

@adriancole Please give me the maintainer list you guys want to add into the npm package. I will add them, and remove myself immediately you guys confirmed privileges are all OK.

jcchavezs commented 5 years ago

@agreatfool awesome. It is enough to add openzipkin as maintainer. Really thanks for the collaboration.

agreatfool commented 5 years ago

Welcomed :)

Done: 

koa-zipkin git:(master) npm owner add "openzipkin" zipkin-instrumentation-koa
+ openzipkin (zipkin-instrumentation-koa)

Could you guys please check the privilege. If OK, please notify me, I would remove privilege of myself.

jcchavezs commented 5 years ago

I just checked npm and it looks like we have the right access to the package. Thanks a lot @agreatfool this is truly community spirit. I hope you can also jump into this new package as you did a great work in https://github.com/agreatfool/koa-zipkin.

agreatfool commented 5 years ago

That's cool. I have removed myself form the npm package. :)