-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tweaks to ckozak's DetachedSpan & demo usage #247
tweaks to ckozak's DetachedSpan & demo usage #247
Conversation
Generate changelog in
|
: Tracer.createTrace(Observability.UNDECIDED, Tracers.randomId()); | ||
|
||
Optional<String> parentSpanId = trace.top().map(OpenSpan::getSpanId); | ||
OpenSpan openSpan1 = OpenSpan.of( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
openSpan1
-> openSpan
?
|
||
Map<String, String> metadata = Collections.emptyMap(); | ||
Span span = Tracer.toSpan(inProgress, metadata, traceId); | ||
Tracer.notifyObservers(span); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it'd be nice to pull lines 85-89 and 70-76 into a common private function
|
||
// we throw away the existing thread local state and replace it with our view of the world | ||
Tracer.clearCurrentTrace(); | ||
Tracer.setTrace(Trace.of(observable, traceId)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe I'd wrap up the clearCurrentTrace+setTrace into a couple a private method replaceTraceState
that takes no args, you can reuse it in child
, too
} | ||
|
||
/** | ||
* Unusual method - returns a new {@link CrossThreadSpan} parented to this instance. Only necessary if you want |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be helpful to include an example in the docs of when to use this, it's not obvious how to select child
or this method, both result in a parented CrossThreadSpan?
@jellis curious if this works for your needs? |
…o dfox/cross-thread-tracing
Ok so this is just gonna land into the existing DetachedSpan branch - I think it will let us do all the tracing we want in c-j-r. Hope the names look OK from your perspective @carterkozak? Definitely still want to explore Mark's new internal representation ideas, but don't want to block this PR on that (as it should be easy to polyfill later if we need to). |
@@ -21,10 +21,10 @@ | |||
/** | |||
* Closeable marker around a tracing span operation. This object should be used in a try/with block. | |||
*/ | |||
public interface SpanToken extends Closeable { | |||
public interface CloseableSpan extends Closeable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I like this better.
👍 |
1 similar comment
👍 |
Before this PR
In conjure-java-runtime and dialogue, we want to be able to measure a span of time beginning on one thread and ending on another (e.g. backoffs or client-side concurrency limits). We do have an
AsyncTracer
class already, but this makes two spans:<operation>-enqueue
and<operation>-run
.After this PR
A newCrossThreadSpan
class allows measuring operations that begin on one thread and end on another.I'm not expecting this to be used by regular service authors much, but hoping we can benefit from it in our libraries.Possible downsides?
TODO