Merge SpanContext into Span and make DefaultSpan private by anuraaga · Pull Request #1712 · open-telemetry/opentelemetry-java

anuraaga · 2020-09-26T08:47:44Z

While open-telemetry/opentelemetry-specification#999 won't be merged, I think we can say that we have SpanContext, it just happens to be merged into Span as an implementation detail.

The API seems to be vastly simplified by this change. Far less dancing, no creating a Span just to have a container for storing in Context - we always just work with Spans. No need to explain to users what the difference between SpanContext and Context is, we only have one context. How to link to a span? Add a link to the span.

I noticed that we use DefaultSpan for three purposes currently

A propagated span, as in Define Propagation-only Span to simplify active Span logic in Context. opentelemetry-specification#994. I added Span.getPropagated
An unsampled, no-op span. I added Span.getUnsampled, though not sure it's a better name than Span.getNoop
A test span just to use as a holder of context in unit tests. I just used Span.getPropagated for most of these though it's not technically precise, but we could just as easily define a test utility for this and I don't think it's a huge deal

Let me know any thoughts. Will finish up with javadoc if seems ok.

Fixes #1704
Fixes #1135

codecov · 2020-09-26T09:03:12Z

Codecov Report

Merging #1712 into master will decrease coverage by 0.19%.
The diff coverage is 86.03%.

@@             Coverage Diff              @@
##             master    #1712      +/-   ##
============================================
- Coverage     85.48%   85.29%   -0.20%     
- Complexity     1378     1382       +4     
============================================
  Files           164      163       -1     
  Lines          5402     5433      +31     
  Branches        559      566       +7     
============================================
+ Hits           4618     4634      +16     
- Misses          586      595       +9     
- Partials        198      204       +6

Impacted Files	Coverage Δ	Complexity Δ
...pentelemetry/opentracingshim/ScopeManagerShim.java	`72.72% <0.00%> (-9.10%)`	`5.00 <0.00> (ø)`
.../main/java/io/opentelemetry/sdk/trace/Sampler.java	`100.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...telemetry/sdk/trace/export/BatchSpanProcessor.java	`86.00% <0.00%> (-3.34%)`	`8.00 <1.00> (ø)`
...ions/trace/jaeger/sampler/JaegerRemoteSampler.java	`68.62% <0.00%> (ø)`	`9.00 <0.00> (ø)`
...ions/trace/jaeger/sampler/PerOperationSampler.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...opentelemetry/opentracingshim/SpanBuilderShim.java	`54.16% <33.33%> (-0.48%)`	`22.00 <1.00> (ø)`
...telemetry/extensions/trace/propagation/Common.java	`72.22% <50.00%> (ø)`	`10.00 <0.00> (ø)`
...ions/trace/jaeger/sampler/RateLimitingSampler.java	`70.83% <50.00%> (ø)`	`8.00 <3.00> (ø)`
.../main/java/io/opentelemetry/trace/DefaultSpan.java	`70.21% <66.66%> (-10.56%)`	`26.00 <12.00> (+7.00)`	⬇️
api/src/main/java/io/opentelemetry/trace/Span.java	`81.25% <70.00%> (-18.75%)`	`7.00 <7.00> (+7.00)`	⬇️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0969d4...5fe4c94. Read the comment docs.

Oberon00 · 2020-09-26T11:14:02Z

api/src/main/java/io/opentelemetry/trace/Link.java

+  String getTraceIdAsHexString();
+
+  String getSpanIdAsHexString();
+
+  TraceState getTraceState();
+
+  byte getTraceFlags();


This actually seems worse than before. Could we return a DefaultSpan here instead?

Link is strange in being over-coupled to export right now. I don't think export structures should be based on e.g., Span so I'll look at it in #1714.

Oberon00 · 2020-09-26T11:16:29Z

This PR is already huge. I suggest moving the "make DefaultSpan private" to a separate/follow-up PR. You could select this PR's branch as target branch and link it in the PR description. Or ensure it is separate in the commit history of this PR at least.

anuraaga · 2020-09-26T11:42:31Z

@Oberon00 I found a lot of code using SpanContext and DefaultSpan interchangeably. So I think the story of this PR isn't complete without combining both. If the direction is ok, I can split out the DefaultSpan visibility change if others also find it changes the reviewability significantly.

bogdandrutu

This needs to go to the specs, because we remove an important concept from the API

anuraaga · 2020-09-27T01:08:09Z

Yeah but since open-telemetry/opentelemetry-specification#999 is marked after-ga it won't be acted on right? Are we stuck with the cruft because of that? I would still say we haven't really removed anything, it's just a matter of presentation within the Java code.

trask · 2020-09-27T02:35:27Z

opentracing_shim/src/main/java/io/opentelemetry/opentracingshim/SpanContextShim.java

+  private final String traceId;
+  private final String spanId;
+  private final byte traceFlags;
+  private final TraceState traceState;


does it work to store a propagated Span here instead?

trask · 2020-09-27T03:02:49Z

@bogdandrutu can you give us some options for how we can move forward with this before GA?

This looks like a really good change, especially now with Propagated Span, SpanContext class doesn't seem to be pulling any weight.

I also think this is a very good point from @anuraaga:

When users come to tracing for the first time, I think they understand the concept of a span. And they notice that a span is supposed to have a trace id and span id, they see this in tracing UIs. The presence of SpanContext seems to just add indirection that complicates the model for new users. It's especially confusing with a similarly named but very different Context.

trask · 2020-09-27T11:09:15Z

@anuraaga maybe go ahead and send spec PR for open-telemetry/opentelemetry-specification#999? IIRC after-ga means not required for ga, it doesn't preclude a spec PR from being accepted prior to GA, I would think especially a spec PR that doesn't require other SDKs to do any new work

Oberon00 · 2020-09-27T12:18:38Z

Spec is already frozen for non-editorial changes. Maybe you can get an exception for that one though? As it does not increase (required) work for SIGs.

trask · 2020-09-28T04:07:16Z

I added this topic to the maintainers meeting agenda for tomorrow to explore our options

anuraaga · 2020-09-28T04:18:57Z

@trask (I think) it's just a one line change so doesn't hurt to make a PR indeed :) open-telemetry/opentelemetry-specification#1022

iNikem · 2020-09-28T04:31:18Z

Does current spec prevent us from having SpanContext as interface, btw?

anuraaga · 2020-09-28T04:35:21Z

@iNikem Nope that was changed in open-telemetry/opentelemetry-specification#969. So for issues of bridging, I think an interface would be fine too if this can't be accepted. But I realized the API could be made much simpler here while working on that PR :)

iNikem · 2020-09-28T04:39:03Z

If we make SpanContext an interface, and make Span implement it, maybe we can still simplify Java API significantly without direct contradiction to spec?

anuraaga · 2020-09-28T07:26:12Z

@iNikem Didn't think of that idea - seems to be an ok option if SpanContext must survive but a bit of an unnecessary interface. Guess I can start with it to get this out of draft

carlosalberto · 2020-09-29T14:15:24Z

opentracing_shim/src/main/java/io/opentelemetry/opentracingshim/ScopeManagerShim.java

+    // OpenTracing simply returns null when the current span is not valid.
    io.opentelemetry.trace.Span span = tracer().getCurrentSpan();
-    if (DefaultSpan.getInvalid().equals(span)) {
+    if (!span.isValid()) {


Slightly incorrect, as in OT it's possible to set an invalid Span (one with zero-ed ids) as the active instance. We should check against Span.getInvalid()

Do you think you can give me a unit test that fails with the current code? OTel has many invalid checks, for example in instrumentation - I'd be surprised if we could even support a zero'd but live span in OTel. I'm guessing OT would propagate such a span, but OTel won't - so maybe we can just unify this behavior with the OTel semantics? Anyways a unit test would probably make things very clear, I'm not changing it back if we can't get the current code to fail first :)

Another way to phrase my point of confusion - I would think returning a singleton for an invalid span is a performance optimization but wouldn't expect any behavior change. So if the use of a singleton is relied on here, it's unexpected and seems fragile. If it's required I need to add some more detailed docs.

Not supporting valid spans with all zero trace or span ID is something that irks me about OTel actually, see open-telemetry/opentelemetry-specification#753

anuraaga · 2020-10-13T08:06:52Z

Separated out #1791 so closing this

anuraaga requested review from bogdandrutu, carlosalberto, jkwatson and trask September 26, 2020 08:47

Merge SpanContext into Span.

58d60db

anuraaga force-pushed the span-context branch from 3ad25a3 to 58d60db Compare September 26, 2020 08:50

anuraaga requested review from Oberon00 and arminru September 26, 2020 08:56

OTel isn't null

5fe4c94

Oberon00 reviewed Sep 26, 2020

View reviewed changes

anuraaga mentioned this pull request Sep 26, 2020

Link should be implemented in Span, not SpanData #1714

Closed

bogdandrutu requested changes Sep 26, 2020

View reviewed changes

trask reviewed Sep 27, 2020

View reviewed changes

anuraaga mentioned this pull request Sep 28, 2020

Allow SpanContext to be exposed directly on Span. open-telemetry/opentelemetry-specification#1022

Closed

anuraaga mentioned this pull request Sep 29, 2020

Merge SpanContext into Span open-telemetry/opentelemetry-specification#1033

Closed

carlosalberto reviewed Sep 29, 2020

View reviewed changes

anuraaga mentioned this pull request Oct 13, 2020

Remove DefaultSpan from public API. #1791

Merged

anuraaga closed this Oct 13, 2020

Conversation

anuraaga commented Sep 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Oberon00 Sep 26, 2020

Choose a reason for hiding this comment

Uh oh!

anuraaga Sep 26, 2020

Choose a reason for hiding this comment

Uh oh!

Oberon00 commented Sep 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anuraaga commented Sep 26, 2020

Uh oh!

bogdandrutu left a comment

Choose a reason for hiding this comment

Uh oh!

anuraaga commented Sep 27, 2020

Uh oh!

trask Sep 27, 2020

Choose a reason for hiding this comment

Uh oh!

trask commented Sep 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trask commented Sep 27, 2020

Uh oh!

Oberon00 commented Sep 27, 2020

Uh oh!

trask commented Sep 28, 2020

Uh oh!

anuraaga commented Sep 28, 2020

Uh oh!

iNikem commented Sep 28, 2020

Uh oh!

anuraaga commented Sep 28, 2020

Uh oh!

iNikem commented Sep 28, 2020

Uh oh!

anuraaga commented Sep 28, 2020

Uh oh!

carlosalberto Sep 29, 2020

Choose a reason for hiding this comment

Uh oh!

anuraaga Sep 29, 2020

Choose a reason for hiding this comment

Uh oh!

anuraaga Sep 29, 2020

Choose a reason for hiding this comment

Uh oh!

Oberon00 Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anuraaga commented Oct 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

anuraaga commented Sep 26, 2020 •

edited

Loading

codecov bot commented Sep 26, 2020 •

edited

Loading

Oberon00 commented Sep 26, 2020 •

edited

Loading

trask commented Sep 27, 2020 •

edited

Loading

Oberon00 Sep 29, 2020 •

edited

Loading