Skip to content

[improvement] Add AsyncTracer that tracks enqueued-time and run-time of a deferred action - #117

Merged
bulldozer-bot[bot] merged 10 commits into
palantir:developfrom
robert3005:rk/deferred-tracer-perseves-stack
Mar 29, 2019
Merged

[improvement] Add AsyncTracer that tracks enqueued-time and run-time of a deferred action#117
bulldozer-bot[bot] merged 10 commits into
palantir:developfrom
robert3005:rk/deferred-tracer-perseves-stack

Conversation

@robert3005

@robert3005robert3005 commented Mar 29, 2019

Copy link
Copy Markdown
Contributor

Before this PR

DeferredTracer only preserves traceid and last span id

After this PR

Add AsyncTracer that preserves full span stack of parent trace

cc @ellisjoe

@robert3005
robert3005 requested a review from a team as a code ownerMarch 29, 2019 15:59
@carterkozak

Copy link
Copy Markdown
Contributor

Why is it important that we can start a span, then complete it on multiple threads? tracing-java makes assumptions about threads that become foot-guns when applied to asynchronous usage. It might be time for a v2 API.

@ellisjoe

Copy link
Copy Markdown
Contributor

Yeah, we're explicitly interested in tracing async things (eg: how long tasks spend in queues or waiting for their client side limiters)

@carterkozak

Copy link
Copy Markdown
Contributor

Can we produce a new function for this case with fewer sharp edges? I'm worried about queueing multiple operations, and having them all complete copies of the same span. We need some way to enforce the concept of span ownership.

@robert3005

Copy link
Copy Markdown
ContributorAuthor

@cakofony what is the case you currently have that you're worried about? Isn't it the case that multiple threads can finish copies of the span before #97

@carterkozak

Copy link
Copy Markdown
Contributor

Yes, that was a bug and a foot-gun.

Perhaps this behavior should live in a separate @Beta tracer type which doesn't need to be serializable. It's not clear to me that DeferredTracer is the right home for this functionality. We don't want to defer a trace, we're elongating a span.

private final Trace deferredTrace;
private final String operation;
@Nullable
private final String parentSpanId;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go with this approach it will require a serialVersionUID bump

this.parentSpanId = null;
this.operation = null;
}
this.operation = operation.orElse(DEFAULT_OPERATION);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forwarding @dfox's comment about the default operation name: #110 (comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed this sorry!

@carterkozak

Copy link
Copy Markdown
Contributor

Rough notes on the detached span idea so I remember later:

/* Does not modify thread state, the current span is used as detached spans parent * if it exists, otherwise a detached span is created with a new trace that has no * thread state. */DetachedSpandetached = Tracer.startDetachedSpan("operation")
interfaceDetachedSpan {
/* Executes the Runnable with tracing state applied. */voidrun(Stringoperation, Runnablerunnable)
/* Executes the Callable with tracing state applied. */
<T> Tcall(Stringoperation, Callable<T> callable)
/* Complete this span */voidclose();
}

@robert3005
robert3005force-pushed the rk/deferred-tracer-perseves-stack branch from 442b77b to 81b71d7CompareMarch 29, 2019 17:54
@robert3005robert3005 changed the title [improvement] Preserve full span stack in DeferredTracer[improvement] Add AsyncTracer that preserves full span stack of parent trace Mar 29, 2019
@carterkozak

Copy link
Copy Markdown
Contributor

Thanks @robert3005 👍

Comment threadtracing/src/main/java/com/palantir/tracing/Tracer.java Outdated
@bulldozer-bot
bulldozer-botBot merged commit f9a91b6 into palantir:developMar 29, 2019
@dansanduleacdansanduleac changed the title [improvement] Add AsyncTracer that preserves full span stack of parent trace [improvement] Add AsyncTracer that tracks enqueued-time and run-time of a deferred actionMar 29, 2019
private final Trace deferredTrace;
private final String operation;

public AsyncTracer() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this is a new class, we should have made the operation required.

@iamdanfoxiamdanfox mentioned this pull request Aug 2, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@robert3005@carterkozak@ellisjoe@dansanduleac@markelliot@pkoenig10