Proof of Concept of "Typed Spans" #195

tylerbenson · 2020-02-29T19:44:43Z

Primary goal is to maintain a simple and semantic interface for the code using it. See TypedTracerDemonstration for example usage.

It's proposed that these would replace the current decorator hierarchy.

[skip ci] Primary goal is to maintain a simple and semantic interface for the code using it. See `HttpClientTracerDemonstration` for example usage.

…act at build time. [skip ci]

tylerbenson · 2020-02-29T21:58:39Z

agent-tooling/src/test/groovy/io/opentelemetry/auto/typed/HttpClientTracerDemonstration.java

+
+import io.opentelemetry.context.Scope;
+
+class HttpClientTracerDemonstration {


See here for example usage.

prydin · 2020-03-03T01:49:14Z

My $0.02:
I like it. Great separation of concern. Spans become self-decorating and you concentrate all the knowledge on how a span should behave to the span itself. The Tracer knows everything about creating and naming spans. Nice and clean!

This is also great for testability. The entire instrumentation can easily be unit tested completely decoupled from any bytecode manipulation.

I'm assuming your aim is eventually to move all the typed spans to opentelemetry-java and turning the auto-project into more or less a bunch of Advice classes that delegate all the actual work to the typed spans and tracers. I love that!

My only VERY minor concern is that there may be a lot of boilerplate code to write for defining the tracer and span types for each instrumentation.

trask · 2020-03-03T05:42:48Z

agent-tooling/src/test/groovy/io/opentelemetry/auto/typed/HttpClientTracerDemonstration.java

+      // span.onResponse("response instance"); // implicitly called on end.
+      span.end(response);
+    } catch (Exception ex) {
+      span.end(ex);


I like end() taking either response or throwable, makes usage very clear 👍

trask · 2020-03-03T05:55:38Z

agent-tooling/src/test/groovy/io/opentelemetry/auto/typed/HttpClientTracerDemonstration.java

+    SampleHttpServerTypedTracer tracer = new SampleHttpServerTypedTracer();
+
+    SampleHttpServerTypedSpan span = tracer.startSpan("request instance");
+    // span.onRequest("request instance"); // implicitly called on start.


Can onRequest() and onResponse() be protected so it's not confusing whether or not they need to be called?

Ok, I redid the package structure to allow this... I had to put the tracer and the span in the same package for it to work.

trask · 2020-03-03T06:04:49Z

agent-tooling/src/main/java/io/opentelemetry/auto/typed/span/DelegatingSpan.java

+  @Override
+  public void end() {
+    delegate.end();
+  }
+
+  @Override
+  public void end(EndSpanOptions endOptions) {
+    delegate.end(endOptions);
+  }


It would be nice if BaseTypedSpan didn't have these two end() methods, so that we could have some typed span hierarchies where they don't exist and it's super clear that you have to call either end(response) or end(throwable)

The problem is it extends from Span, which allows it to be put into Scope as-is. The alternative would be to override the method and throw an exception.

Good point. I want to say maybe override and mark @Deprecated so that IDEs will give hint that you shouldn't be calling it, but @Deprecated isn't quite right, any idea some other way to get IDEs to hint that you shouldn't call it?

Oh, I found example where I'd seen this kind of thing before, Guava's ImmutableList:

/** * Guaranteed to throw an exception and leave the list unmodified. * * @throws UnsupportedOperationException always * @deprecated Unsupported operation. */ @Deprecated @Override public final void add(int index, E element) { throw new UnsupportedOperationException(); }

I think it would be helpful to add this kind of override for end() and end(endOptions) methods in ClientTypedSpan and ServerTypedSpan to help avoid confusion of which end(...) method to call.

My extra comment above isn't showing down here, so copying it:

Although API is not supposed to throw exceptions, and we're hoping these might someday be there, so maybe mark deprecated, but no-op instead of throw exception?

Although API is not supposed to throw exceptions, and we're hoping these might someday be there, so maybe mark deprecated, but no-op instead of throw exception?

agent-tooling/src/main/java/io/opentelemetry/auto/typed/span/HttpClientTypedSpan.java

trask · 2020-03-03T06:23:58Z

agent-tooling/src/main/java/io/opentelemetry/auto/typed/tracer/ClientTypedTracer.java

+import io.opentelemetry.trace.Span;
+
+public abstract class ClientTypedTracer<
+        T extends ClientTypedSpan<T, REQUEST, RESPONSE>, REQUEST, RESPONSE>


yeah, there wasn't a good way to ensure the return types were strongly typed without some extra generics complexity.

agent-tooling/src/main/java/io/opentelemetry/auto/typed/span/HttpClientTypedSpan.java

tylerbenson · 2020-03-03T19:16:31Z

@prydin for the amount of code that would need to be written for each integration, see the SampleHttp*Typed* classes. That is similar to the effort required currently for the decorators.

I think there's still some work remaining to ensure that the API is flexible enough for all our needs, but I think it's a good starting point.

trask · 2020-03-03T19:19:33Z

FWIW, boilerplate doesn't bother me (I'm probably just numb 😁)

…lity. And move setter method to tracer.

[skip ci]

trask · 2020-03-03T20:12:35Z

agent-tooling/src/main/java/io/opentelemetry/auto/typed/base/BaseTypedSpan.java

+    super.end();
+  }
+
+  protected abstract T self();


This doesn't look like it's used anymore

I think it will still be necessary if we override any methods that return T.

trask · 2020-03-03T20:21:12Z

agent-tooling/src/main/java/io/opentelemetry/auto/typed/span/DelegatingSpan.java

+  @Override
+  public void end() {
+    delegate.end();
+  }
+
+  @Override
+  public void end(EndSpanOptions endOptions) {
+    delegate.end(endOptions);
+  }


Oh, I found example where I'd seen this kind of thing before, Guava's ImmutableList:

/** * Guaranteed to throw an exception and leave the list unmodified. * * @throws UnsupportedOperationException always * @deprecated Unsupported operation. */ @Deprecated @Override public final void add(int index, E element) { throw new UnsupportedOperationException(); }

I think it would be helpful to add this kind of override for end() and end(endOptions) methods in ClientTypedSpan and ServerTypedSpan to help avoid confusion of which end(...) method to call.

My extra comment above isn't showing down here, so copying it:

Although API is not supposed to throw exceptions, and we're hoping these might someday be there, so maybe mark deprecated, but no-op instead of throw exception?

tylerbenson · 2020-03-05T20:59:36Z

Related to open-telemetry/opentelemetry-java#964

Oberon00 · 2020-03-12T17:27:36Z

agent-tooling/src/main/java/io/opentelemetry/auto/typed/server/ServerTypedSpan.java

+    super(delegate);
+  }
+
+  protected abstract T onRequest(REQUEST request);


I would not provide this method and instead force the user to provide the request already in startSpan, so that all attributes are set on the builder and thus available to samplers (and onStart callbacks of SpanProcessors)

This method is not exposed to the user... it's intended for implementation by the concrete class. Sounds like you think it's not necessary though?

trask

Discussed in weekly call. This looks like a good place to start iterating from. Thanks @tylerbenson!

codefromthecrypt · 2020-04-13T01:28:01Z

agent-tooling/src/test/groovy/io/opentelemetry/auto/typed/TypedTracerDemonstration.java

+    SampleHttpServerTypedSpan span = tracer.startSpan("request instance");
+    // span.onRequest("request instance"); // implicitly called on start.
+
+    try (Scope scope = tracer.withSpan(span)) {


suggestion: make a static class Request and Response as tracer.startSpan("request instance"); looks very much like a normal start of a span with a pre-canned name

Proof of Concept of "Typed Spans"

c425e22

[skip ci] Primary goal is to maintain a simple and semantic interface for the code using it. See `HttpClientTracerDemonstration` for example usage.

tylerbenson requested a review from trask February 29, 2020 19:44

Add additional startSpan methods and restructure to allow tracer.extr…

e7a42aa

…act at build time. [skip ci]

tylerbenson commented Feb 29, 2020

View reviewed changes

tylerbenson mentioned this pull request Mar 2, 2020

Auto and Contrib sharing common functionality #WIP #148

Closed

trask reviewed Mar 3, 2020

View reviewed changes

tylerbenson added 2 commits March 3, 2020 11:54

Move classes into a different package structure to enable less visibi…

3ff8321

…lity. And move setter method to tracer.

Add deprecation warnings to unused end methods

b2d6f51

[skip ci]

trask reviewed Mar 3, 2020

View reviewed changes

trask mentioned this pull request Mar 5, 2020

Share code and tests with contrib instrumentation #45

Closed

Oberon00 reviewed Mar 12, 2020

View reviewed changes

tylerbenson added 2 commits March 12, 2020 14:47

Remove nullable.

b82f271

Add headers.

49a6aa2

tylerbenson marked this pull request as ready for review March 12, 2020 22:17

tylerbenson requested review from jkwatson and safris as code owners March 12, 2020 22:17

Merge branch 'master' into tyler/POC-typed-tracers

0873848

trask approved these changes Mar 13, 2020

View reviewed changes

trask merged commit f5c5e57 into master Mar 13, 2020

trask deleted the tyler/POC-typed-tracers branch March 13, 2020 01:33

This was referenced Mar 13, 2020

Prototype to show how Typed Spans can be implemented open-telemetry/opentelemetry-java#964

Closed

"Typed Spans" is confusingly named #229

Closed

Instrumentation framework does not encourage use of SpanBuilder-set attributes #230

Closed

codefromthecrypt reviewed Apr 13, 2020

View reviewed changes

iNikem mentioned this pull request Jun 1, 2020

Http spans #466

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proof of Concept of "Typed Spans" #195

Proof of Concept of "Typed Spans" #195

tylerbenson commented Feb 29, 2020 •

edited

Loading

tylerbenson Feb 29, 2020

prydin commented Mar 3, 2020

trask Mar 3, 2020

trask Mar 3, 2020

tylerbenson Mar 3, 2020

trask Mar 3, 2020

tylerbenson Mar 3, 2020

trask Mar 3, 2020

trask Mar 3, 2020 •

edited

Loading

trask Mar 3, 2020

trask Mar 3, 2020

tylerbenson Mar 3, 2020

tylerbenson commented Mar 3, 2020

trask commented Mar 3, 2020

trask Mar 3, 2020 •

edited

Loading

tylerbenson Mar 3, 2020

trask Mar 3, 2020 •

edited

Loading

tylerbenson commented Mar 5, 2020

Oberon00 Mar 12, 2020

tylerbenson Mar 12, 2020

trask left a comment

codefromthecrypt Apr 13, 2020


		import io.opentelemetry.context.Scope;

		class HttpClientTracerDemonstration {

Proof of Concept of "Typed Spans" #195

Proof of Concept of "Typed Spans" #195

Conversation

tylerbenson commented Feb 29, 2020 • edited Loading

Choose a reason for hiding this comment

prydin commented Mar 3, 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

Choose a reason for hiding this comment

trask Mar 3, 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

tylerbenson commented Mar 3, 2020

trask commented Mar 3, 2020

trask Mar 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trask Mar 3, 2020 • edited Loading

Choose a reason for hiding this comment

tylerbenson commented Mar 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trask left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerbenson commented Feb 29, 2020 •

edited

Loading

trask Mar 3, 2020 •

edited

Loading

trask Mar 3, 2020 •

edited

Loading

trask Mar 3, 2020 •

edited

Loading