0.32.0 Release

[carlosalberto]

This issue tracks the Release Candidate iterations towards the the new 0.32 API for OpenTracing-Java.

RC Branch: https://github.com/opentracing/opentracing-java/tree/v0.32.0

Current Status

• Trace Identifiers have been added (SpanContext.toTraceId() and SpanContext.toSpanId()).
• Finishing Span upon Scope.close() has been deprecated.
• SpanBuilder.startActive() has been deprecated.
• Scope.span() has been deprecated.
• ScopeManager.activeSpan() has been added.
• Small refactoring of the BINARY format (now we let the users know about the required buffer size).
• Other API sugar for making the developers' life easier ;)

Changes that might be included for the final release

• ScopeManager.clear()
• GlobalTracer registration improvements.
• MockTracer improvements.

Release Process

• The new API is being developed on the v0.32.0 branch.
• Changes to the release candidate will be released on maven as 0.32.0.RC1, 0.32.0.RC2, etc.

[carlosalberto]

Hey everybody!

I created this Issue as a way to track the incoming RC iterations towards the 0.32.0 Release (just as we did with the previous 0.31 one). Feel free to comment and leave any feedback on this. The plan is to do a first Release Candidate on the next days ;)

cc @yurishkuro @opentracing/opentracing-java-maintainers @felixbarny

[codefromthecrypt]

I understand how deprecations on api signatures work. I don't understand how one would accomplish a deprecated behavior like this:

"Finishing Span upon Scope.close() has been deprecated."

What does this mean? We still finish on close but somehow people know this won't happen later? If so, I disagree with this solution as it will just delay problem. Or are you deprecating methods that allow this? Maybe this can be clarified?

[codefromthecrypt]

Initial is that you should very quickly release a v0.33.0 which removes the deprecated methods entirely.

I'll add some feedback based on thoughts in the diff v0.31.0...v0.32.0

• ScopeManager#active() -- still there and undeprecated.. it should not be there
• Span#setTag(AbstractTag<T> key, T value) and SpanBuilder#withTag(AbstractTag<T> key, T value)-- very very awkward to make an api require an abstract class as a parameter.
• SpanContext#toTraceId() and SpanContext#toSpanId() -- very awkward to return empty string as opposed to null on unsupported. Probably should have considered a subtype. Also the to prefix is also awkward
• Binary type feels strange, it has ByteBuffer bias to it which I think we discussed in the past (ex is this the right carrier? how would you know). Also there's some misspelling in there. It was added eventhough there were a lot of troubled comments on the Simple layer on top of ByteBuffer for BINARY format. #276
• BinaryAdapters has the same code smell as TextMap recently re-discussed in Separate TextMap into read/write interfaces. #315 which is that you can see it isn't entirely natural to combine things into a single type. I would revert both

In general, there seems some care about deprecating, but not removing methods we know are malpractice. However, there's certainly api break here. If you can't remove the crappy methods, I would consider releasing an overlay with the crappy methods we are moving off of, placing that burden of carrying them on the maintainers of the spec as opposed to every tracer author.

[carlosalberto]

Hey @adriancole

We still finish on close but somehow people know this won't happen later? If so, I disagree with this solution as it will just delay problem.

We don't want to deliberately break the API from version to version - the plan has been, all along, to let final users know this API will get removed (I'd suggest for 0.33). As part of this we will also strongly discourage them to use the current behavior, as it makes it hard to report errors - but we want them do be able to have an incremental update.

Else, we will be forcing tracer code and instrumentation frameworks code and instrumented application code to be updated, along with significant refactoring if this new version wants to be used (observe this version will not only include this change, so, if users want to use any of the other features/fixes, they will be forced to port their code all at once).

[carlosalberto]

ScopeManager#active() -- still there and undeprecated.. it should not be there

Based on #301 it was agreed that we will leave it in place, as some users may still need it, specially when there's no place/context to store the Scope (as mentioned in #267 (comment) ). That's why we added ScopeManager.activeSpan() and indirectly why Scope.span() was deprecated too (so users do not pass Scope around).

Span#setTag(AbstractTag key, T value) and SpanBuilder#withTag(AbstractTag key, T value)-- very very awkward to make an api require an abstract class as a parameter.

Personally I don't find it to be a problem, specially because we require an AbstractTag but we already expose three implementations of it. But I will leave others jump in here.

SpanContext#toTraceId() and SpanContext#toSpanId() -- very awkward to return empty string as opposed to null on unsupported. Probably should have considered a subtype. Also the to prefix is also awkward

There was extensive discussion on this topic in #280 about it, and the names were chosen, as much as you may not like them, for two reasons: 1) to signal that returning them as a String may incur in allocating memory to convert them from their native representation, and 2) to avoid collisions with tracer already exposing trace/span id with different signature. Hey, we won't want to make their life hard and break the API for them and their users ;) If you have a better name, feel free to propose it, though.

Binary type feels strange, it has ByteBuffer bias to it which I think we discussed in the past (ex is this the right carrier? how would you know). Also there's some misspelling in there. It was added eventhough there were a lot of troubled comments on the #276

So in #276 we had quite a few iterations - if you have any input about the misspelling, let me know. Other than that, the change is rather simply: we have a current BINARY format that is essentially a pain to use, as we cannot tell the user the required size at Tracer.inject() time. And this change essentially helps with that.

We had also extensively discussed the need for a more advanced binary format - should it be byte[] based, or ByteBuffer based, or stream based? There was not enough information at the moment, and it wasn't agreed at anything as shown here: #252 (comment) - and besides, I mentioned that Cassandra on the server side receives custom payloads as a simple ByteBuffer (#252 (comment)) and also, while prototyping support for Hadoop, I mentioned having something to simply use a ByteBuffer for injection/extraction could be useful (the rather ugly but working prototype lies here: https://github.com/carlosalberto/hadoop/commits/ot_initial_integration)

I see no reason to NOT have a more advanced format when/if somebody needs it required - meanwhile, having something that works at a basic level would help.

BinaryAdapters has the same code smell as TextMap recently re-discussed in #315 which is that you can see it isn't entirely natural to combine things into a single type. I would revert both

That can probably be done - will play with it myself after we issue a first RC (nothing is set in stone at this point).

[carlosalberto]

That being said, I'd like to hear what other people have to said about this one, and I'd still go ahead and do a first RC for 0.32, at it will allow users to try out and test this new API - so realize how it feels. As already said: nothing is set in stone, even if we do a RC this week ;)

[yurishkuro]

Regarding abstract class, we can replace it with an interface, eg TypedTag.

[yurishkuro]

+1 for RC

[tedsuo]

I'd also like to see this move forwards as an RC, so that tracers will binding to it and we can get more feedback.

I like the idea of providing a way to remove the deprecated methods, similar to https://github.com/opentracing/opentracing-java-v030, so that tracers can stop having to maintain these methods but users can continue to update their tracers. But I think there's also value in an API version which still contains the deprecated methods so that users can migrate incrementally without having to switch their import statements.

[carlosalberto]

@tedsuo I think we can mention that for 0.33 we will be removing them altogether - the v030 package made more sense as it was an important-but-required API breaking change. Also IHMO it's a bad idea to be changing so much the API from version to version.

Anyway, will create a small PR with the changes @yurishkuro mentioned, and after merging them I will do the RC1 (and we will keep on iterating on it, towards a second RC2 perhaps, in case we need it).

[pavolloffay]

We should also include #289 into the next release. @carlosalberto could you please have a look?

[carlosalberto]

@pavolloffay Sure, will be checking it prior to do our RC1 ;)

[felixbarny]

SpanContext#toTraceId() and SpanContext#toSpanId() -- very awkward to return empty string as opposed to null on unsupported.

What are the pros and cons of returning an empty string?
Pros

• Adding an MDC entry where the value is null leads to exceptions for some loggers. Returning an empty string guards against this and still allows to omit adding an MCD entry if empty.
• Guards against NPEs when dereferencing the string

Cons

• It's somewhat weird
• May lead to parsing errors when trying to parse the id as hex. It's however unsafe to assume the string is a hex encoded byte array.

Probably should have considered a subtype.

Not sure I follow, subtype of what?

Also the to prefix is also awkward

The to prefix indicates that this is not a simple accessor but that it may cause allocations/conversions from the internal representation (which may be a byte[] or long) to a string.

// @adriancole