Skip to content

RFC only! (ActiveSpanManager etc)#1

Open
bhs wants to merge 4 commits intomasterfrom
bhs/tls_etc
Open

RFC only! (ActiveSpanManager etc)#1
bhs wants to merge 4 commits intomasterfrom
bhs/tls_etc

Conversation

@bhs
Copy link
Copy Markdown
Owner

@bhs bhs commented Feb 20, 2017

Just some commentary...

tedsuo and others added 3 commits February 17, 2017 16:34
WIP
Still Missing:
* MockTracer and TestTracer must be implemented
* Exectors should be subclassed with traced wrapper methods
  https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/Executors.html
* Some tests mebbe
Copy link
Copy Markdown
Owner Author

@bhs bhs left a comment

Choose a reason for hiding this comment

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

(just some commentary)

/**
* XXX(bhs): comment
*/
public interface ActiveSpanManager {
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Split out from the Tracer. Also note the Snapshot sub-interface.

/**
* XXX: comment
*/
void deactivate(Snapshot snapshot);
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I don't love that this requires a Snapshot (rather than just a Span), but doing a Span has bad implications for the impl.

/**
* XXX: comment
*/
public class MDCActiveSpanManager implements io.opentracing.ActiveSpanManager {
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

proving to myself that this can work well with MDC and the context map.

void finish();

// XXX: comment
boolean isFinished();
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

this is necessary if we want to avoid Tracer/Span wrappers (which I kinda want to avoid)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💯


public class TracedExecutorService implements ExecutorService {
private ExecutorService executor;
private ActiveSpanManager manager;
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

note how this thing does not need a Tracer ref anymore, and it's also pretty easy to write given an ASM.

* <li>If no single tracer service is found, the {@link io.opentracing.NoopTracer NoopTracer} will be used.</li>
* </ol>
*/
public final class GlobalTracer implements Tracer {
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

(didn't touch this)

this.parentId = parent.spanId;
}

if (tracer.getActiveSpanManager() != null) {
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

would need to do something to get around all of these checks. I think it's going to be racy to have the ASM settable at something other than construction-time for a Tracer.

public interface ActiveSpanManager {
// Basically a marker interface
public interface Snapshot {
Span span();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants