Add finagle client instrumentation

[seglo]

Instrumentation to manage Kamon spans for finagle client request & response lifecycle.

Relates to #238

[seglo]

I think the project is referenced in the right places of the build.sbt, but I don't see its test results in the CI build.

[ivantopo]

Hey @seglo, awesome that you are contributing this instrumentation! I'm going to be away until Monday, then I'll take a proper look 👀

[seglo]

@ivantopo Thanks! Do you have some time to review this week?

[ivantopo]

Hey @seglo, I'm on it now 😄

Got covid last week and it really kept me busy, but now I'm back. Here are a few questions that came up during my first look at it:

1. Is there any particular reason why only the client side is instrumented? We can definitely start just with the client, but I was wondering if there is anything big preventing from bringing full support.
2. Is there anything preventing us from adding the Kamon filter automatically via automatic instrumentation? so that people don't need to do anything by hand to get tracing.

[ivantopo]

It shouldn't be necessary to add the span kind as a tag. The kind has its own field on the spans and it is set automatically by the HTTP Client instrumentation, and then the reporters turn it into a tag when the backend needs it. Here I don't think it is necessary.

[seglo]

Got it. Yes, I see span.kind being added to tags.

The span type is an interesting case. We use the kamon-datadog span reporter and I noticed that the DDSpan that's serialized and sent to DataDog hardcodes its span kind field to custom. There are a handful of supported options. They have subtle effects on how the APM frontends are rendered. We've since overridden this by implementing our own KamonDataDogTranslator to send it as web. I was thinking about opening a Kamon issue for this to make it more configurable, but our solution seems to work fine for now.

I'll remove this tagging from the implementation. I can sort out what our infrastructure needs outside of this instrumentation.

[ivantopo]

I think that this is something we definitely should address in the datadog reporter itself. It wouldn't be too complicated to check if a span has SpanKind=Client|Server and some http.* tag, we would automatically add the span.type tag there so it works nicely in Datadog's UI.

Also, I remember an old conversation about showing DB spans properly in Datadog. It might be a similar thing we need to do. Let's track those changes in a separate issue/pr: #1098

[ivantopo]

There are a few tags that I don't recall using in the past, could you please share what made you include them? I'm talking about:

• span.type
• error.type
• http.status_category

[seglo]

I addressed span.type in the other comment.

error.type this is something we use internally, but now that I think about it it's something we can instead define as a mark when errors occur for certain finagle operation events, so I'll make that change and drop the tag.

http.status_category another internal concern I'll remove from this implementation.

[ivantopo]

Great! please ping me when you add those changes to look at this again.

[seglo]

Thanks for the review @ivantopo !

Got covid last week and it really kept me busy, but now I'm back. Here are a few questions that came up during my first look at it:

Oh my goodness. I hope you're recovering well!

1. Is there any particular reason why only the client side is instrumented? We can definitely start just with the client, but I was wondering if there is anything big preventing from bringing full support.

My team uses Akka for most of our needs, which already has good coverage by Kamon. However, much of the rest of our infrastructure uses finagle http and gRPC services that we integrate with. Those services have a separate custom Open API finagle tracing instrumentation and they use DataDog libraries instead of Kamon.

A server implementation is totally do-able, but I'll have to discuss it with the team to see if we can spare the time to work on it since we don't have a ready made solution. That said, many parts of the implementation required for adding server support can be reused from what's already in this PR so it may not be too much work and something I can tackle in my free time.

2. Is there anything preventing us from adding the Kamon filter automatically via automatic instrumentation? so that people don't need to do anything by hand to get tracing.

It may be possible to set a default Finagle tracer. I'll have to do some digging.

[ivantopo]

Hey @seglo, never mind about the server part then. I thought that you were using the server as well but maybe it was complicated to instrument and I could help you push it forward 😄, if you are not really using it then better to wait until someone using the server-side of Finagle comes along and wants to give it a try.

[ivantopo]

Hey @seglo, I'm really lacking on any Finagle-specific knowledge to comment on that part of the work, but Kamon-wise this seems good to go. Do you have any additional comments or changes to include before merging?

[seglo]

better to wait until someone using the server-side of Finagle comes along and wants to give it a try.

Agreed! If the situation changes and we need to maintain one of our finagle services then I might follow up then if someone else hasn't already implemented server-side support.

Hey @seglo, I'm really lacking on any Finagle-specific knowledge to comment on that part of the work, but Kamon-wise this seems good to go. Do you have any additional comments or changes to include before merging?

@ivantopo I think it's ready to go, but I'll update with master and do one more pass first. Are you cutting a release soon? I can probably get to this later today.

[ivantopo]

I have one more tiny change to include and then ship a release, so probably it will all go out tomorrow morning if you manage to do those last checks today. And if it doesn't go tomorrow we can do another release with this PR alone!

[seglo]

@ivantopo Ok. IMO it's good to go!