Closed bg451 closed 8 years ago
Is it interesting to make this use a vanilla standardtracer
? Because I (tentatively) think you could if standardtracer exposed some hooks.
For example, most stuff that happens on the appdash.Recorder
can be deferred until Finish()
. If you hid the appdash.Recorder
in an internal tag, you could have access to it then via a hook. For getting the recorder in, we'd need a hook that gets called with the old and new trace during a fork. Just thinking.
@tschottdorf
Is it interesting to make this use a vanilla standardtracer? Because I (tentatively) think you could if standardtracer exposed some hooks.
For example, most stuff that happens on the appdash.Recorder can be deferred until Finish(). If you hid the appdash.Recorder in an internal tag, you could have access to it then via a hook. For getting the recorder in, we'd need a hook that gets called with the old and new trace during a fork. Just thinking.
I am in favor of not using standardtracer
for a few reasons at this point:
@bg451 IIRC on Slack you said a significant portion of this code comes copy+pasted from the standardtracer implementation itself, and I've got a lot of style concerns that I don't want to berate you with here. Would it be better for me to raise them in opentracing-go repository instead?
Also, awesome work on this! I think this will be very valuable long term :)
@slimsag Yeah sure, fyi all the standardtracer code was moved into opentracing/basictracer-go. I'll go ahead and start making some changes on your feedback for now.
Could you take a look at whether https://github.com/opentracing/basictracer-go/pull/5 allows you to do the same thing effectively without a copy of basictracer?
@tschottdorf Not quite. It gets most of it, but there's some special sugar surrounding span creation that deals with appdash.Recorder
.
Basically, we fork off either the parent span's Recorder
or the tracers Recorder
and overwrite the Recorder.SpanID
field to hold the new state. The reason for this is to maintain the reference of the Recorder
s internal appdash.Collector
.
This is a bit offtopic, but when @slimsag and I initially discussed the implementation of this, it made sense to have a top level appdash.Recorder
inside the tracer, however I would be open to having a top level appdash.Collector
instead and creating a new appdash.Recorder
since we're essentially repeating the steps in appdash.NewRecorder
.
@bg451 hmm, too bad. So you basically need at creation time of the child access to out-of-band state in the parent's EventManager
? StartSpanWithOptions
is where all of that tricky stuff happens, correct?
@slimsag Check out the new commits when you get the chance. The new implementation uses basictracer now and cut's out all of the copy-pasted code.
This is ready for review. I'll squash the commits when everything is good to go.
/cc @slimsag
Apologies it's taken me so long to get around to this @bg451! This is some really great work! The basictracer implementation is by far a lot cleaner and I'm 100% convinced this is the right way to go for now.
LGTM once you address my comments :)
Comments addressed, PTAL when you get the chance @slimsag.
I'm sure there will be more to do here as I start to play with this and get a better feel for how all the components fit together in real-world applications, but this looks like a superb starting point and I'm excited to try this out once I have some spare time :musical_note:
Thank you so much for this contribution, and for bearing with my long-winded review, @bg451 !
@slimsag It's been a pleasure!
In terms of things to do, the first thing that comes to mind is getting important events working. I'll save that discussion for later, though.
This is the Appdash implementation of the OpenTracing api for Go. This is a really big diff, so I'd recommend ignoring the tests and the example app that's included for the time being. cc @slimsag