Rename 'io.jaegertracing.Span' to 'io.jaegertracing.JaegerSpan'

[jpkrohling]

The Span class clashes with OpenTracing's Span, which is confusing when debugging and reading code like:

public interface Sender {
  int append(Span span) throws SenderException;
}

It's also not clear what is part of the public API and what is not.

I suggest renaming the class io.jaegertracing.Span to io.jaegertracing.internal.JaegerSpan. This class would still extend io.opentracing.Span. Usage of JaegerSpan would be allowed only by internal components and will not be part of the public API.

This means that the Span from the sender example above would be OpenTracing's, as well as Reporter#report(Span).

The same applies to Tracer and SpanContext.

[pavolloffay]

Then it probably applies to more classes e.g. Tracer, SpanContext

[yurishkuro]

Doesn't Java support renaming as part of the import? Maybe I am confusing it with Scala.

[yurishkuro]

cc @vprithvi

[pavolloffay]

Doesn't Java support renaming as part of the import? Maybe I am confusing it with Scala.

I don't think so

[yurishkuro]

Can we instead use some linter that will require fully qualified names for Span class usage?

[jpkrohling]

Can we instead use some linter that will require fully qualified names for Span class usage?

I think it would be a step in the wrong direction, to be honest. As part of #435, these classes wouldn't be public. Ideally, we'd consume and expose only OpenTracing's Span, SpanContext and Tracer. One notable exception would be the JaegerTracer that will be an interface with an embedded builder.

[jpkrohling]

By the way: JaegerSpan would still extend Span so that consumers can still consume them as OpenTracing's Span.

[jpkrohling]

I just updated the description of this issue with the current state of the discussion.

[jpkrohling]

I prepared a branch with a draft based on this:
https://github.com/jaegertracing/jaeger-client-java/compare/master...jpkrohling:434-RenameSpanTracerSpanContext?expand=1

@pavolloffay, @yurishkuro, @vprithvi, would this be something desirable? The renaming itself is only part of the story now, I believe.

[yurishkuro]

I think it would be a step in the wrong direction, to be honest. As part of #435, these classes wouldn't be public. Ideally, we'd consume and expose only OpenTracing's Span, SpanContext and Tracer.

That might be problematic. Some users may depend on more detailed API of Jaeger span than the OpenTracing span. There are certainly users who cast to Jaeger span today in order to read trace/span ID, and sampling state, which weren't available via OT. I am all in favor of tightening the API though.

[jpkrohling]

Some users may depend on more detailed API of Jaeger span than the OpenTracing span.

We need something different, then. We need an interface that serves as the contract, telling which methods they can rely on. Something like:

interface JaegerSpan extends io.opentracing.Span
class JaegerBaseSpan implements JaegerSpan

interface JaegerSpanContext extends io.opentracing.SpanContext
class JaegerBaseSpanContext implements JaegerSpanContext

Where we today accept/return Jaeger's Span/SpanContext, we'd start accepting/returning JaegerSpan/JaegerSpanContext, which serves as our public API. This way, we can expose public long getTraceId() as part of the public API, but not public static SpanContext contextFromString(String).

There are certainly users who cast to Jaeger span today in order to read trace/span ID, and sampling state, which weren't available via OT

Those are part of the SpanContext, but your point is valid. In any case, they can still do that, as the concrete type will still be JaegerSpan/JaegerSpanContext. But returning OT's Span tells the consumer that that's what is "supported" and casting to a more specific type might get them in trouble in the future.

[yurishkuro]

I am still not sold on this renaming being a good thing. It creates redundancy in the naming that is already being addressed by the package namspacing provided by the language. Why do we need to go to C-style naming?

There are going to be clear domains in the code where only one import makes sense, so all others can be prohibited via linter. For example, why is there a need to ever used unqualified io.opentracing.Span? The full name is not long, and it is namespaced.

[jpkrohling]

In most of the cases, the consumer code will only have to deal with OpenTracing's Span/SpanContext/Tracer, so, whatever names we use internally won't matter. If they do decide to use an implementation-specific class, then it's far more readable to have JaegerSpan vs. io.jaegertracing.Span.

I thought we were past the phase where we discussed whether this was a good thing or not?

[isaachier]

What about backwards-compatibility where the current io.jaegertracing.Span is being used?

[jpkrohling]

What about backwards-compatibility where the current io.jaegertracing.Span is being used?

This is a good point. This change will break the compatibility with clients currently referencing io.jaegertracing.Span: they would have to start using io.jaegertracing.JaegerSpan instead.

I think we would break them anyway with #396, though. For instance, we'll probably remove the io.jaegertracing.Span#getTracer() from the public API.

[yurishkuro]

If we keep the interface name as io.jaegertracing.Span, we can potentially avoid these problems. If we only call the internal span JaegerSpan, then I have less concerns with this renaming.

[jpkrohling]

If this is a blocking concern, we can settle on io.jaegertracing.Span vs. io.jaegertracing.JaegerSpan, but I'll insist that it's not idiomatic in Java to have io.jaegertracing.Span [extends|implements] io.opentracing.Span as it is in other languages.

In fact, on #439 @vprithvi suggested using sonarcloud for code quality analysis. Interestingly, one of the "code smells" that it reports is about this class name:

https://sonarcloud.io/project/issues?id=io.jaegertracing%3Ajaeger-client&moduleUuids=AWPWXE7K4427HkKVyzHU&open=AWPWXFDX4427HkKVyzQ0&resolved=false

To recap, looks like we have the following contributors in favor of the rename of io.jaegertracing.Span to io.jaegertracing.JaegerSpan:

• @pavolloffay
• @objectiser

And the following against:

• @yurishkuro (blocking concern?)
• @isaachier

I would also really appreciate feedback from other contributors as well. @black-adder? @vprithvi?

[yurishkuro]

Would sonarcloud still complain if io.jaegertracing.Span was an interface? And does it actually have to implement io.opentracing.Span? It is meant to provide a different kind of API (i.e. in Scala I would totally make it a small trait with just the Jaeger-specific methods, not an extension of the main API).

[jpkrohling]

Would sonarcloud still complain if io.jaegertracing.Span was an interface?

I'll try that out and report back

And does it actually have to implement io.opentracing.Span?

That's a great question, actually. I would say that it's indeed desirable, as it makes it easier to pass spans around, casting to a specific type whenever necessary.

For instance, our Tracer#activeSpan() returns OpenTracing's, but our Reporter#report(Span) requires a io.jaegertracing.Span. Right now, we just pass the same io.jaegertracing.Span to the caller as a io.opentracing.Span.

If they don't belong to the same type hierarchy, we need a converter, like there's between Thrift's Span and Jaeger's

[yurishkuro]

Are you thinking of having the reporter accept the new public interface? The reporter will need access to full span details, not just the minimal methods exposed on the interface.

[jpkrohling]

Are you thinking of having the reporter accept the new public interface? The reporter will need access to full span details, not just the minimal methods exposed on the interface.

The PR actually goes one step further and makes reporters accept an OpenTracing's Span. Turns out, only senders need Jaeger-specific details and even there, it's converted into a Thrift Span.

[yurishkuro]

I commented on the PR about that - don't think that's a good direction. Which brings back to my question above.

[jpkrohling]

Agree, I'll revert that part. I'm not sure yet which signature the method should have after we define the internal API. I was originally thinking about setting it to report(JaegerSpan), so that people implementing their own reporters could rely on the interface and we'd have the freedom to add/change internal methods on the implementation. I think this is a discussion we can have on that future PR, though.

[jpkrohling]

Would sonarcloud still complain if io.jaegertracing.Span was an interface?

Yes: