opentracing-java

opentracing-java copied to clipboard

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.

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

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?

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 https://github.com/opentracing/opentracing-java/compare/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 https://github.com/opentracing/opentracing-java/pull/276
• 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

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.

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).

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 https://github.com/opentracing/opentracing-java/issues/267#issuecomment-382126090 ). 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<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.

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: https://github.com/opentracing/opentracing-java/pull/252#issuecomment-388456367 - and besides, I mentioned that Cassandra on the server side receives custom payloads as a simple ByteBuffer (https://github.com/opentracing/opentracing-java/pull/252#issuecomment-384983611) 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).

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 ;)

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

+1 for RC

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.

@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).

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

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

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

I agree on an empty string being awkward. Ideally I think it should be an Optional<String> otherwise I'd propose it returns null and we include guard methods hasTraceId hasSpanId and indicate that it should be invoked before attempting to get the Ids. Either way the user can decide what to do, whether they'll use an empty string or whatever else.

@felixbarny I'll comment.. most of the bias is being least weird especially in apis meant to be like standard libraries.

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.

I'm struggling to understand why we are designing an api that's purpose is for consumers including MDC, and choosing to use empty string as a signal of absence. Can you cite anything with the same rationale as we have that does this? If not, what would convince you against being, not just somewhat, but blatantly and intentionally weird.

Not sure I follow, subtype of what?

I mean if you want to hide that there are no IDs in there, you could consider a type like RealSpanContext that has accessors that are never null, and don't put those same ones on the base type. I don't like it, but it is less awkward than empty string. RealSpanContext as a type is just for consideration, I still think null is far more important. I don't think people should blanket add empty strings into things like this. It is much cheaper to add nothing and to do that is similar code to being null and less awkward way to represent absence.

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.

please cite things? I'm convincible on this one. I just think there's way too little concern for prior art in this. toXX is not common, and why should a user care really if there was a conversion? Is there an alternative? In a similar feature I have see xxxAsString() to highlight this. Just asking for dilligence and references by default. That would end quite a lot of the types of questions I sometimes ask.

to predict a question on " It is much cheaper to add nothing" by anyone, not just @felixbarny

do your own benchmark, and choose bench checking null vs invoking MDC commands blindly with an empty string. the things that invoke MDC with a trace ID are going to be limited, ideally standard interceptors! the scope of folks who need to do things like this.. ideally they can look at a signature for this like all other things they look at signatures (or don't look at signatures). They will have the null MDC problem with or without our hack that only covers trace/span ID. This is my conjecture.

SkyWalking will keep on OT 0.30, consider we can't make span across thread. If and only if we could see something with span in single thread guarantee and explicit across thread context notification mechanism, upgrade is possible.

I'm struggling to understand why we are designing an api that's purpose is for consumers including MDC, and choosing to use empty string as a signal of absence.

Not sure if that even was the reason for the empty string. I just tried to start a conversation about the pros and cons of this approach :). @carlosalberto could you clarify the reason for empty string vs null?

Ideally I think it should be an Optional otherwise I'd propose it returns null and we include guard methods hasTraceId hasSpanId and indicate that it should be invoked before attempting to get the Ids

Optional would be great here but the OT API supports Java 6 so that would not be an option. I'd rather check for non-null than checking hasSpanId first tbh.

Reading through the PR again (https://github.com/opentracing/opentracing-java/pull/280#issuecomment-408285398) the to prefix was mainly chosen to not break existing tracer impls. Why can't they just rename their internal methods?

We could theoretically add additional methods like Optional<String> traceIdOptional() which can then only be called from a Java 8+ codebase. That actually works without breaking pre-Java 8 code in obvious ways. There are corner cases though. Doing reflection on the Span interface would throw errors.

@felixbarny

could you clarify the reason for empty string vs null?

I do not remember all the details, but I remember the starting points were 1) To follow the specification (which mentions empty strings as valid values) and 2) that returning a string instead of null would be safer.

I'm sure that if others have a strong feeling against it, so we could have a PR to have it moved to null (I personally do not have a preference, as both options have good and bad things, as previously mentioned).

I'd rather check for non-null than checking hasSpanId first tbh.

I agree on this. Adding extra methods for doing the same does not give us much IHMO.

Why can't they just rename their internal methods?

The problem was more about public methods (even if they were not covered by the OT api).

could you clarify the reason for empty string vs null?

Actually, this reasoning is mentioned in the spec: https://github.com/opentracing/specification/blob/master/rfc/trace_identifiers.md#tracer-support ;)

Some existing tracers may not be able to support this feature, as their internal model does not include any client-side trace identifiers. These tracers may choose to not support this feature by returning empty string values.

This is still no reasoning why empty strings are preferred to null values. @tedsuo can you clarify?

My interpretation of that would mean that a Tracer might have an internal representation of a Scope/SpanContext/Span that does not have an ID, this does not preclude returning null, when none of these exist in the current context. And actually, that use case stregthens the case for returning null, since if you return an empty string you have no way of being certain that there is not an active context

If you're already adding SpanContext.toTraceId() and SpanContext.toSpanId(), is there a strong reason for not adding SpanContext.toParentId()? Would be quite helpful to have, and would allow me to totally scratch all vendor dependent code from java-jfr-tracer.

Also, what is the ETA for 0.32.0? :)

@thegreystone He. I think we'd like to iterate over the existing items the incoming week (for adjusting/updating the API depending on the feedback), and then perhaps issue a second RC (that would be happening in 2 weeks, I'd say). If everything goes smooth, then a pair of weeks after that we would be releasing 0.31 ;) (if not, we might need a pair of weeks more to iterate again on the features).

Thanks Carlos! (You mean 0.32, right?) What about toParentId()?

We have never discussed parent id. What is the use case for accessing it?

For building the DAG (directed acyclic graph) of spans.