Add Span#unwrap

[felixbarny]

With Span wrappers becoming more and more popular (for example https://github.com/opentracing-contrib/java-api-extensions). There is a growing need to be able to get access to the underlying Span implementation without type casts which fail if there is a hierarchy of span wrappers.

[coveralls]

Coverage decreased (-0.1%) to 78.467% when pulling 87a120e on felixbarny:unwrap into cdc9601 on opentracing:v0.31.0.

[coveralls]

Coverage increased (+0.2%) to 78.832% when pulling f259b89 on felixbarny:unwrap into cdc9601 on opentracing:v0.31.0.

[yurishkuro]

based on the two implementations (Mock and Noop), not clear how this is different from cast - is the intention that wrappers will override this method?

[felixbarny]

Yes, a wrapper's implementation might look like this:

public <T extends Span> T unwrap(Class<T> clazz) {
	if (clazz.isInstance(this)) {
		return clazz.cast(this);
	} else {
		return delegate.unwrap(clazz);
	}
}

where delegate is a Span instance variable

[jpkrohling]

Or, if Java 8+ is the baseline, the interface can implement a basic unwrap routine, freeing up implementations to have code that would be mostly duplicated.

[sjoerdtalsma]

At first glance it looks as though we 'pollute' Span with a concern that may not be applicable to all spans.
Maybe define a SpanWrapper interface for this that all wrappers can then implement? This keeps Span cleaner.

interface SpanWrapper extends Span {
    <T extends Span> T unwrap(Class<T> spanType);
}

[sjoerdtalsma]

SpanWrapper could be part of util

[felixbarny]

That kind of defeats the whole purpose of the PR. Because then it is still not possible to unwrap a plain Span to a specific one without type casts.

By the way, this is heavily inspired by JDBC wrappers: https://docs.oracle.com/javase/7/docs/api/java/sql/Wrapper.html

[sjoerdtalsma]

I know. 😉
I'm also not a fan of the unwrapping of DataSources this way.

In my opinion unwrapping is not something that is of use for the general API; people have a Tracer and 'just use it'. For tracer implementors things are different and it would be very easy for them to create an (internal) static utility method such as

static <T extends Span> T unwrap(Span span, Class<T> spanType) {
    return span instanceof SpanWrapper ? ((SpanWrapper) span).unwrap(spanType) : null;
}

The point of this PR should be (in my opinion) a standardized way to indicate that you can unwrap. For this the interface serves the purpose.

For me not mixing concerns and having a clean api outweighs typechecks any day.

Just my opinion of course. Curious what others think.

[felixbarny]

But how can we enforce then that each tracer's Span implements the SpanWrapper interface?

[sjoerdtalsma]

But how can we enforce then that each tracer's Span implements the SpanWrapper interface?

You can't unfortunately.
But we can add documentation to the util module with explanation why it's sensible to implement SpanWrapper when wrapping. Add PR's to 'visible examples' such as java-api-extensions

[tylerbenson]

I don't think this adds significant value to the general API. If the wrap fails, then it just returns null. That seems like a worse place to be. Not to mention, this will just encourage people to write code that is specific to a tracer implementation, rather than the OT api. At that point, why pretend to use the OT api at all? Just work directly with the tracer's concrete types all the way through.

[jpkrohling]

I think this is a good improvement to the API. Other successful APIs, like JPA, have such methods and it's really handy to have them when you absolutely need to have access to an implementation-specific feature.

Not to mention, this will just encourage people to write code that is specific to a tracer implementation, rather than the OT api

I don't think it will encourage. People are already doing casts to get the underlying trace IDs.

[sjoerdtalsma]

I don't think it will encourage. People are already doing casts to get the underlying trace IDs.

Which points to the root cause: An omission of a general concept in the OpenTracing api: Trace ID's.

I still say: Let people typecast at their on risk but not facilitate this. Needing typecasts is usually a sign that a needed concept is still missing in the core API.
Providing an 'easy' unwrap will miss requests for such concepts and people will work around the API more than improving it. Issues such as this are a good thing and help improve the actual specification rather than help work around it.

By the way: I agree that it would be hugely beneficial for the end user if there were a single place to go get an opaque trace ID, I just don't think that polluting the API with unwrapping is the way to go.

[jpkrohling]

Which points to the root cause: An omission of a general concept in the OpenTracing api: Trace ID's.

Trace IDs was just one example of why people are doing that today.

Needing typecasts is usually a sign that a needed concept is still missing in the core API.

It's very common for features to flow from implementations into the standard, like it has happened multiple times in the JPA world (from EclipseLink/Hibernate to JPA).

People will cast to the concrete implementation when they need, no matter if we provide a unwrap method or not. Providing this method will just make their code cleaner. Nothing more, nothing less.

[pavolloffay]

I think it's useful, and especially if wrappers are used, otherwise there is no way to properly cast the object.

[tedsuo]

Now that we are moving on Trace Identifiers, we should consider adding unwrap as part of the same release. A couple thoughts:

• We should probably move this to a cross-language discussion, since other language API's will probably need something similar.
• Should we be unwrapping Tracers as well?
• Even if we don't want unwrap "as-is", I feel we should not ship trace identifiers without some kind of solution for this issue.

[pavolloffay]

Should we be unwrapping Tracers as well?

+1, it would help in MicroProfile-OpenTracing TCK and other use cases. The test deployment uses io.opentracing.Tracer which is casted to MockTracer in an endpoint which sends data back to the test. Also see point 3 eclipse/microprofile-opentracing#58 (comment)

Unwrap would be also helpful in GlobalTracer

[sjoerdtalsma]

Unwrap would be also helpful in GlobalTracer

...which was specifically designed to provide only the API, so people are not tempted to work around it.

I'm still 👎 on adding unwrap until I have found a use case that cannot be solved by a proper API extension

[pavolloffay]

@sjoerdtalsma you might have missed microprofile-opentracing use case altogether...

A reasonably well designed API shouldn't force you to do reflectio, like it is currently with globaltracer API. If a caller does dummy thins it's his issue. Look at other APIs e.g. JPA or hystrix plugins...

[sjoerdtalsma]

@pavolloffay I'm genuinely willing to be convinced.
When you say "you might have missed microprofile-opentracing use case altogether" do you mean your comment

it would help in MicroProfile-OpenTracing TCK

.. and the reference point 3 you mentioned:

The TCK mentions the use of Mock Tracer, but doesn't actually show how to use it (critical for the running file)

Probably I'm missing something, but from the above I still don't get what the use-case for unwrap is here..

I think @tylerbenson explained better than me when he wrote
#211 (comment):

I don't think this adds significant value to the general API. If the wrap fails, then it just returns null. That seems like a worse place to be. Not to mention, this will just encourage people to write code that is specific to a tracer implementation, rather than the OT api. At that point, why pretend to use the OT api at all? Just work directly with the tracer's concrete types all the way through.