Implement spec change to only accept Context as span parent. #1611

Oberon00 · 2020-09-01T15:00:17Z

Partial implementation of (now merged) open-telemetry/opentelemetry-specification#875

This is a prototype for spec PR open-telemetry/opentelemetry-specification#875 (spec issue open-telemetry/opentelemetry-specification#510).

To be more minimal, it does not currently add the Context parameter to SpanProcessor.OnStart, but that would be a trivial follow-up.

OLD OUTDATED DESCRIPTION

It replaces the parentSpanId + hasRemoteParent in RecordEventsReadableSpan with a full io.grpc.Context. ~~That was the simple part. The part that is a bit funky is that DefaultSpan now has also has a parent Context in addition to still having a SpanContext for itself.~~

All in all, this removes 24 lines according to git diff --stat, so as stated in the spec PR, the total complexity does not really increase here.

The most interesting changes are in SpanBuilderSdk, RecordEventsReadableSpan ~~and DefaultSpan~~.

END OLD OUTDATED DESCRIPTION

codecov · 2020-09-01T15:12:22Z

Codecov Report

Merging #1611 into master will increase coverage by 1.02%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1611      +/-   ##
============================================
+ Coverage     85.36%   86.38%   +1.02%     
+ Complexity     1382     1378       -4     
============================================
  Files           164      162       -2     
  Lines          5418     5349      -69     
  Branches        556      554       -2     
============================================
- Hits           4625     4621       -4     
+ Misses          594      521      -73     
- Partials        199      207       +8

Impacted Files	Coverage Δ	Complexity Δ
...ain/java/io/opentelemetry/trace/DefaultTracer.java	`97.22% <ø> (-0.40%)`	`5.00 <0.00> (ø)`
api/src/main/java/io/opentelemetry/trace/Span.java	`100.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...opentelemetry/opentracingshim/SpanBuilderShim.java	`54.63% <100.00%> (+0.95%)`	`22.00 <0.00> (ø)`
...ava/io/opentelemetry/sdk/trace/SpanBuilderSdk.java	`96.90% <100.00%> (+1.10%)`	`32.00 <10.00> (-7.00)`	⬆️
...opagation/B3PropagatorInjectorMultipleHeaders.java	`91.66% <0.00%> (-8.34%)`	`4.00% <0.00%> (ø%)`
...tensions/trace/propagation/OtTracerPropagator.java	`78.12% <0.00%> (-4.64%)`	`10.00% <0.00%> (ø%)`
.../propagation/B3PropagatorInjectorSingleHeader.java	`95.65% <0.00%> (-4.35%)`	`5.00% <0.00%> (ø%)`
...io/opentelemetry/exporters/otlp/MetricAdapter.java	`90.44% <0.00%> (-1.87%)`	`26.00% <0.00%> (+1.00%)`	⬇️
...extensions/trace/propagation/JaegerPropagator.java	`86.41% <0.00%> (-1.09%)`	`26.00% <0.00%> (ø%)`
...ntelemetry/exporters/prometheus/MetricAdapter.java	`89.85% <0.00%> (-0.15%)`	`15.00% <0.00%> (ø%)`
... and 11 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 10ea529...332ce25. Read the comment docs.

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

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

anuraaga

I feel this is conflating two unrelated ideas, the parent / child relationship of spans (a logical connection of IDs) and a mechanism for propagating data within a process, often across threads (Context). The end result is contexts have spans, and spans also have contexts - it's not quite circular but it's not a logical relationship I think.

If someone needs to propagate data along with the span, there's no need to do it with as part of parent/child relationship I think, the data can be added to the Context next to the span, e.g., after creating the span, Context.withValue(valuetopropagate, Context.current()). If we really need a simpler API for adding arbitrary information to go along with the span (that is how I'm reading the rationale in #875), we could have a first-class field for extra data like Brave. But I find it confusing if we try to use the span parent for this sort of data.

sdk/tracing/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java

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

sdk/tracing/src/main/java/io/opentelemetry/sdk/trace/data/SpanData.java

Oberon00 · 2020-09-02T07:41:43Z

@anuraaga

I feel this is conflating two unrelated ideas, the parent / child relationship of spans (a logical connection of IDs) and a mechanism for propagating data within a process, often across threads (Context).

Please, can we discuss such fundamental design concerns on the spec PR open-telemetry/opentelemetry-specification#875?

anuraaga · 2020-09-02T09:48:46Z

I realized that context APIs don't allow iterating over the context, so my concern of leaking unrelated information into the exporter isn't really a concern.

So still about lifecycle, how about an example with the root span. Want to propagate some information with it.

Context context = Context.current().withValue(cat, meow);
Span span = tracer.spanBuilder().parent(context).start();

I want to add data to the span, so I have to add it to the context. And I add a parent even though it's a root span. This API seems very confusing to me. Perhaps builder has helpers to set the values, but then this would create an internal context forked from current when building the span that probably can't be mounted. I think we would have to change start to return Context instead of Span with such an approach to make clear that the old context needs to be invalidated by propagating this new one. This might make some sense and makes me more comfortable with the change, but it seems like a huge change too.

anuraaga · 2020-09-02T09:49:47Z

Doh sorry wrong PR!

…rent Conflicts: api/src/main/java/io/opentelemetry/trace/DefaultTracer.java

…rent Conflicts: exporters/jaeger/src/test/java/io/opentelemetry/exporters/jaeger/AdapterTest.java exporters/zipkin/src/test/java/io/opentelemetry/exporters/zipkin/ZipkinSpanExporterEndToEndHttpTest.java exporters/zipkin/src/test/java/io/opentelemetry/exporters/zipkin/ZipkinSpanExporterTest.java sdk/tracing/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java sdk/tracing/src/main/java/io/opentelemetry/sdk/trace/SpanBuilderSdk.java sdk/tracing/src/test/java/io/opentelemetry/sdk/trace/SpanBuilderSdkTest.java testing_internal/src/main/java/io/opentelemetry/sdk/trace/TestSpanData.java

jkwatson · 2020-09-15T16:27:15Z

@Oberon00 What would you think about narrowing the scope of this PR to only update the API, rather than also change it to move the Context all the way down into the exporter layer? I think your spec issue has had the scope narrowed recently to only include the API changes.

Oberon00 · 2020-09-15T18:07:45Z

I think your spec issue has had the scope narrowed recently to only include the API changes.

Indeed, this was changed in today's SIG spec meeting. I will update the PR (will probably remove more than half the changes) but I didn't get around to it yet.

…rent

Oberon00 · 2020-09-16T09:50:58Z

I updated the PR. Currently it is even more minimal than the spec PR (no Context parameter for OnStart), but that would be trivial follow-up.

jkwatson · 2020-09-16T16:26:58Z

I updated the PR. Currently it is even more minimal than the spec PR (no Context parameter for OnStart), but that would be trivial follow-up.

thanks!

Oberon00 · 2020-09-23T14:43:48Z

This is now ready to merge, since the spec PR was merged. Note that a follow-up is required to add the new Context parameter for SpanProcessor.OnStart.

jkwatson · 2020-09-23T16:58:09Z

api/src/test/java/io/opentelemetry/trace/SpanBuilderTest.java

@@ -32,8 +33,8 @@
  void doNotCrash_NoopImplementation() {
    Span.Builder spanBuilder = tracer.spanBuilder("MySpanName");
    spanBuilder.setSpanKind(Kind.SERVER);
-    spanBuilder.setParent(DefaultSpan.getInvalid());
-    spanBuilder.setParent(DefaultSpan.getInvalid().getContext());
+    spanBuilder.setParent(TracingContextUtils.withSpan(DefaultSpan.create(null), Context.ROOT));


youch. this API sure is ugly! Remind me again how this is going to make things easier/better for our users?

This is a unit test. Normally you would just call setNoParent.

fair enough, but I think having to do parenting via the TCU is going to be pretty counter-intuitive to most users. :(

I fully agree, but at the same time, I don't think users should normally do that. Usually you would just use the implicit current parent, or sometimes you need to capture a context with Context.current() and forward that to somewhere else. If you start a span at one thread and need to continue it at another thread while not setting it as active in the current thread is the only place you would need TracingContextUtils. I also think there should be some may to make that more easy. I could imagine span.inCurrentContext() (Java 8 default methods?), or TracingContextUtils.spanInCurrent(span) to make this easier.

Remind me again how this is going to make things easier/better for our users?

As you said, using TracingContextUtils is rather unintuitive, and thus I think most users did not do it, they just passed around spans without the remaining context. However, a custom propagator should be able to set a value in the context in extract and rely on it still being available in inject.

BTW, using inject is a place where you already have to often muck around with TracingContextUtils currently.

Is it safe to assume the implicit current parent is accurate?

For instance, when dealing with asynchronous methods the thread being executed on is usually not the same thread that might have initiated/received the original request. In that situation, the implicit current parent from ThreadLocal is more than likely not the parent you want

Maybe we can provide some convenience function (default interface implementation?), so one can do Context capturedParent = span.withCurrentContext(). But it would be interesting how common the cases where you would need this even are.

Personally, it's a lot messier, but more importantly way less obvious that that's what is needed to achieve the desired result.

With respect to a default interface implementation, that wouldn't be possible with the JDK 7 requirement, correct?

(FWIW, we need more convenience functions in TracingContextUtils, or in whatever new name it ends up having ;) )

@kenfinnigan Java 7 was dropped recently, Java 8 is now required.

Thanks @Oberon00, don't follow things for a few weeks, and everything changes!

...pentelemetry/sdk/extensions/trace/testbed/concurrentcommonrequesthandler/RequestHandler.java

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

…rent Conflicts: sdk/tracing/src/main/java/io/opentelemetry/sdk/trace/SpanBuilderSdk.java

jkwatson

Thanks!

anuraaga

Thanks @Oberon00!

Prototype for Context as span parent.

8154b32

Oberon00 mentioned this pull request Sep 1, 2020

Spans's parent must be passed as Context instead of Span(Context)s. open-telemetry/opentelemetry-specification#875

Merged

Oberon00 commented Sep 1, 2020

View reviewed changes

api/src/main/java/io/opentelemetry/trace/Span.java Outdated Show resolved Hide resolved

Oberon00 commented Sep 1, 2020

View reviewed changes

api/src/main/java/io/opentelemetry/trace/DefaultTracer.java Outdated Show resolved Hide resolved

Oberon00 added 3 commits September 1, 2020 18:55

Simplify ZipkinSpanExporterEndToEndHttpTest.

a7fccf4

Minimize PR diff.

e102def

Make Codecov happy.

5500b81

Oberon00 changed the title ~~Prototype for Context as span parent.~~ [RFC] Prototype for Context as span parent. Sep 1, 2020

Oberon00 marked this pull request as ready for review September 1, 2020 18:17

Oberon00 requested review from anuraaga, arminru, bogdandrutu, carlosalberto, jkwatson, pavolloffay, thisthat and tylerbenson as code owners September 1, 2020 18:17

anuraaga reviewed Sep 2, 2020

View reviewed changes

Fix overcomplicated DefaultTracer.

0eb9829

Oberon00 mentioned this pull request Sep 2, 2020

DefaultTracer: Fix noParent being ignored after explicit parent. #1617

Merged

Oberon00 added 4 commits September 2, 2020 19:34

Merge remote-tracking branch 'upstream/master' into feature/contextpa…

e152a50

…rent Conflicts: api/src/main/java/io/opentelemetry/trace/DefaultTracer.java

Fix build after merge.

66b6130

Merge branch 'master' into feature/contextparent

2221c2a

Oberon00 mentioned this pull request Sep 11, 2020

Move the TraceFlags isSampled boolean into the SpanContext #1628

Merged

Merge branch 'master' into feature/contextparent

c24deaf

Fix build after merge.

30fbaf7

Oberon00 added 2 commits September 16, 2020 11:21

Merge remote-tracking branch 'upstream/master' into feature/contextpa…

255cd9c

…rent

Revert storage of full Context.

2253f8d

Oberon00 added 2 commits September 23, 2020 16:20

Merge branch 'master' into feature/contextparent

2e275b2

Lint.

e8dc898

Oberon00 changed the title ~~[RFC] Prototype for Context as span parent.~~ Implement spec change to only accept Context as span parent. Sep 23, 2020

Oberon00 requested a review from anuraaga September 23, 2020 14:49

jkwatson reviewed Sep 23, 2020

View reviewed changes

...pentelemetry/sdk/extensions/trace/testbed/concurrentcommonrequesthandler/RequestHandler.java Outdated Show resolved Hide resolved

jkwatson reviewed Sep 23, 2020

View reviewed changes

opentracing_shim/src/main/java/io/opentelemetry/opentracingshim/SpanBuilderShim.java Outdated Show resolved Hide resolved

Oberon00 mentioned this pull request Sep 24, 2020

Check if we should preserve more of the Context in the OpenTracing shim. #1699

Closed

Oberon00 added 3 commits September 24, 2020 17:46

Clean up OpenTracing SpanBuilderShim.

9f618e0

Rename Context -> RequestHandlerContext in testbed.

6eea962

Merge remote-tracking branch 'upstream/master' into feature/contextpa…

332ce25

…rent Conflicts: sdk/tracing/src/main/java/io/opentelemetry/sdk/trace/SpanBuilderSdk.java

jkwatson approved these changes Sep 24, 2020

View reviewed changes

anuraaga approved these changes Sep 25, 2020

View reviewed changes

anuraaga merged commit 83b9ba5 into open-telemetry:master Sep 25, 2020

arminru deleted the feature/contextparent branch October 9, 2020 15:16

Oberon00 mentioned this pull request Oct 12, 2020

Update SpanProcessor.OnStart to receive parent Context. #1785

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement spec change to only accept Context as span parent. #1611

Implement spec change to only accept Context as span parent. #1611

Oberon00 commented Sep 1, 2020 •

edited

Loading

codecov bot commented Sep 1, 2020 •

edited

Loading

anuraaga left a comment

Oberon00 commented Sep 2, 2020

anuraaga commented Sep 2, 2020

anuraaga commented Sep 2, 2020

jkwatson commented Sep 15, 2020

Oberon00 commented Sep 15, 2020

Oberon00 commented Sep 16, 2020

jkwatson commented Sep 16, 2020

Oberon00 commented Sep 23, 2020

jkwatson Sep 23, 2020

Oberon00 Sep 23, 2020

jkwatson Sep 23, 2020

Oberon00 Sep 23, 2020

kenfinnigan Sep 24, 2020

Oberon00 Sep 24, 2020 •

edited

Loading

kenfinnigan Sep 24, 2020

carlosalberto Sep 25, 2020

Oberon00 Sep 25, 2020

kenfinnigan Sep 25, 2020

jkwatson left a comment

anuraaga left a comment

Implement spec change to only accept Context as span parent. #1611

Implement spec change to only accept Context as span parent. #1611

Conversation

Oberon00 commented Sep 1, 2020 • edited Loading

codecov bot commented Sep 1, 2020 • edited Loading

Codecov Report

anuraaga left a comment

Choose a reason for hiding this comment

Oberon00 commented Sep 2, 2020

anuraaga commented Sep 2, 2020

anuraaga commented Sep 2, 2020

jkwatson commented Sep 15, 2020

Oberon00 commented Sep 15, 2020

Oberon00 commented Sep 16, 2020

jkwatson commented Sep 16, 2020

Oberon00 commented Sep 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oberon00 Sep 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkwatson left a comment

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

Oberon00 commented Sep 1, 2020 •

edited

Loading

codecov bot commented Sep 1, 2020 •

edited

Loading

Oberon00 Sep 24, 2020 •

edited

Loading