Context Tags, HTTP Propagation and HTTP Server Instrumentation

[ivantopo]

So folks, here is a bunch of work in progress that I would like to start reviewing. The most important parts I think are:

• The addition of context tags which adds a few more ways of creating a context. Also, context tags come to replace what the previous "broadcast strings" were doing so I completely removed them.
• The introduction of an HTTP propagation mechanism. This is a replacement to the previous codecs functionality. Before, the codecs for HTTP were based on TextMaps, which in turn were a leftover from the OpenTracing days. Now the HTTP propagation just needs header readers and header writers to work and additionally has the notion of "propagation direction", the common directions where incoming and outgoing but we are also seeing a rising number of requests for the "returning" direction to propagate things like trace IDs back to the clients and we didn't have any way to do that before other than explicitly adding code for that on each HTTP server instrumentation. Entry readers/writers come to replace the previous codecs. The HTTP propagation should take care of everything related to read/write context tags and entries from now on.
• The introduction of an HTTP server instrumentation. This builds on top of the HTTP propagation and the already existent tracing infrastructure to make simpler and consistent experience that we can replicate across all instrumentations. It should be fixing Enforce HTTP instrumentation functionality #525. This is not something strictly necessary at the moment, the only motivation to do it right away was to ensure that all the layers will play nicely with each other (context -> http propagation -> http instrumentation).

There are also some code changes like spreading the big Kamon object into different files or sub modules and making sure that users can provide the classloader to be used when creating codecs or anything from a FQCN (meant to be there because of Play dev mode).

I will stop all the other changes and concentrate on the three items above, I might even completely remove those changes from this PR to keep it clean and address those concerns after merging this.

Very much looking forward to comments on this :)

[dpsoft]

what if also we add withSpanID(...)?

[ivantopo]

We could add this, but there is one little thing that we should take into account here: we have the "join with parent span id" thingy which is basically done only for compatibility with Zipkin.. Take a look at kamon-io/kamon-akka-remote#15 for more context.

I'm going to do some tests with the very latest Zipkin, I have this vague idea that it would just work if client/server side had different span ids. Will also ask in their gitter.

[codefromthecrypt]

sorry I meant to reply here. Yes you can choose to not share a span ID and fork a child. The span detail view won't be as useful as the client/server details won't be grouped together. At some point someone could try to make this work by heuristically looking at the parent of a server to see if it is a client then grouping as if they had the same ID cc @zeagord

[ivantopo]

Thanks for the info, Adrian!

One thing that I was thinking to add was some sort of pre/post processors for Spans and maybe we can stay doing the right thing for Zipkin, but only on the spans with span.kind=server without it being a big exception in the main span builder path.

The main motivation for the pre-processors was the ability to copy certain tags from the context into the Span... that is something that this PR is bringing but only for the HTTP server spans and if there is some sort of correlation ID or customer ID, that's most likely something you want to add to all spans, not just the server side. If we have a pre-processor for this, we can as well have a pre-processor for reusing the SpanID when "zipkin mode" is on. I'll play a bit with this idea and see how it goes.

On the post-processor side, I remember having a conversation with Bogdan from OC back in May where he suggested moving metrics collection to some sort of post-finish hook on the Spans which might make things simpler and easier to extend around Kamon's tracer. Although, I wouldn't like to introduce such changes until we gather more use cases for it.

[dpsoft]

I understand that a cleaning is missing :)

[dpsoft]

@ivantopo I left some comments but awesome work bro!!!

[codefromthecrypt]

no guarantee of different span ids

…

On Mon, Sep 17, 2018 at 6:17 PM Ivan Topolnjak ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In kamon-core/src/main/scala/kamon/trace/Tracer.scala <#552 (comment)>: > @@ -158,6 +159,11 @@ object Tracer { this } + def withTraceID(identifier: IdentityProvider.Identifier): SpanBuilder = { We could add this, but there is one little thing that we should take into account here: we have the "join with parent span id" thingy which is basically done only for compatibility with Zipkin.. Take a look at kamon-io/kamon-akka-remote#15 <kamon-io/kamon-akka-remote#15> for more context. I'm going to do some tests with the very latest Zipkin, I have this vague idea that it would just work if client/server side had different span ids. Will also ask in their gitter. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#552 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD617otOHGAvsZc5eeSJTIirGeycG6qks5ub3angaJpZM4WZXyC> .

[ivantopo]

hey @adriancole, not sure I understood your point there :/

[dpsoft]

I think that would be a good idea support (in a future) multiples Readers/Writers for the same key for example:

 incoming {
       span =[ "kamon.trace.SpanPropagation$B3", "kamon.trace.SpanPropagation$B3Single"]
 }

maybe after merge #551 WDYT?

[dpsoft]

@ivantopo looks good to me, I think that we can merge this and then #553 and #551

[varsharun]

Do we know when this feature will be released? We are using kamon-play-2.6, kamon-akka-http-2.5 for our application. The requirement we have right now is that for play we require status_code along with http_method and the same goes with akka-http. Await the above merge to continue with new versions.

[ivantopo]

Hello @varsharun, even though this PR might be merged soon (it should have happened last week) I'm not sure of how soon the Play and Akka HTTP instrumentation will start using these capabilities.. one of the issues that are currently stopping us from moving forward is updating those other modules. I already have branches of a few projects that have been updated, for example, here is the play upgrade although it is not 100% migrated... I'll keep working on those modules and once we can confirm that there are no major problems on actually instrumenting frameworks with theses abstractions then we will merge and publish.

At least there is an upside, when we merge, all the important modules will be already updated :D

[ivantopo]

I think this is good to be merged now, but would like to point a few missing features from this PR that will be addressed separately:

• Copying tags from the context into Spans: copy tags from the context into spans #556
• Tracking remote services calling an instrumented service Tracking of remote services calling an instrumented service #557
• Copying context keys into HTTP responses Copy certain context keys into the HTTP responses #558

Hope we will be moving forward with those in the very near future.

[dpsoft]

@ivantopo LGTM!!