"Typed Spans" is confusingly named

[Oberon00]

(following up on #195)

Typed spans as originally described in open-telemetry/oteps#25 and now proposed in open-telemetry/opentelemetry-java#964 are a concept that tries to make sure at compile time that semantic conventions are followed / make it easy for the user / instrumentation-writer to follow semantic conventions.

What this SIG currently calls typed spans seems to be actually a framework for instrumentation writers that attempts to make it easy for them / force them to properly think about processing the right inputs at the right time.

I think both the instrumentation framework and the semantic convention helpers are useful concepts, but IMHO the term "typed spans" is already occupied with the latter.

[Oberon00]

Further prior art: https://github.com/danielkhan/opencensus-node-typed-span-sample/blob/master/src/HTTPClientSpan.ts, census-instrumentation/opencensus-specs#257

[thisthat]

I think this can be achieved with the #195 proposal.
For instance, an HTTP client can set its Semantic Attributes when starting the span (in addition to inject the context).

https://github.com/open-telemetry/opentelemetry-auto-instr-java/blob/cecabe522af64aef4ca03b9225d303645b3ff77e/agent-tooling/src/main/java/io/opentelemetry/auto/typed/client/http/HttpClientTypedTracer.java#L27-L31

[Oberon00]

Sure it can. But it does so using the plain Span API, nothing is "typed" in that aspect. Am I missing something?

[thisthat]

My comment was to stress that the two things can coexists and are not mutually exclusive

[Oberon00]

Yeah, I agree. See also my gitter comment: https://gitter.im/open-telemetry/opentelemetry-auto-instr-java?at=5e6a6e6c80cc7b7924be1c2f

[trask]

Hi @Oberon00! Would it be possible for you to join the auto-instrumentation SIG meeting on Thu 3/26 @ 9am Pacific Time to discuss?

When we reviewed the two proposals in last week's SIG meeting with @tylerbenson and @thisthat, these were the only differences that we identified:

#195	open-telemetry/opentelemetry-java#964
TypedTracer	SpanBuilderWrapper
Inject/Extract	user implemented
DelegatingSpan	getRaw() to access underlying span
end(response)	end(statusCode)

(somewhat disorganized meeting notes here)

But it seems like you are seeing a more fundamental difference, that I would very much like to understand, in case there's any way for us to still combine efforts on this.

Thanks,
Trask

[Oberon00]

The typed Spans by @thisthat have the single purpose of making it easy to follow semantic conventions, i.e. setting the right set of attributes with the right value types. Does the instrumentation framework in this repo fulfill the that purpose too? Because I think looking for differences is the wrong approach, looking for commonalities would be more interesting -- I don't think there are many, besides the name.

[Oberon00]

To repeat myself from gitter (https://gitter.im/open-telemetry/opentelemetry-auto-instr-java?at=5e6a6e6c80cc7b7924be1c2f): I don't think the two approaches are alternatives at all. I think the ones implementing derived classes for the base classes provided with the instrumentation framework can use the typed spans by @thisthat in their implementation.

  @Override
  protected SampleHttpClientTypedSpan onRequest(Request r) {
    httpClientSpan.setUserAgent(r.getHeader("User-Agent")); // Using @thisthat's typed span.
   // instead of: delegate.setAttribute("http.user_agent", r.getHeader("User-Agent"));
   // ...
    return this;
  }

There are some things (like the HttpUrlConnection overload from @thisthat's HttpClientSpanWrapperBuilder) that could be moved from that package to a concrete HttpUrlConnection"TypedSpan/Tracer" implementation though, so there is some overlap right now.

[trask]

Hey! I think I understand now!

So open-telemetry/opentelemetry-java#964 is all about the attribute setter methods?

And the goal is to only have one of these "typed spans" per semantic convention. And they are not designed for subclassing. E.g. one final class for http client spans, one final class for http server spans, one final class for database spans, etc?

[Oberon00]

Exactly! It's about providing kind of a "reduced" / strongly typed interface to setAttribute/the SpanBuilder that encourages (but does not force) you to follow semantic conventions.
You would only subclass them if you have a semantic convention that says "all the attributes of this other one plus these", although even then, you might want to operate with multiple typed spans/spanbuilders on the same span instance (which is also possible).

[trask]

👍

I'm thinking we can just refer to the span hierarchy in #195 as "Spans". I'm not sure we need a special name for them.

We can just refer to them as HttpClientSpan, HttpServerSpan, ApacheHttpClientSpan, etc...

If we do need a name for them, maybe we could call them "Instrumentation Spans", since the goal is for these to live under io.opentelemetry.instrumentation and be used by both auto- and contrib- instrumentation.

[iNikem]

I propose to close this issue. We have moved on from the time when it was filed.

#502 proposes typed spans in their original form: classes with setter corresponding to semantic convention. This repo has already for some time used new typed tracers: classes that allow instrumentation writes to set right attributes at the right time. So I think @Oberon00 concern is actually resolved.

[trask]

Closing.

@Oberon00, @thisthat please feel free to comment here, we are happy to re-open and continue discussing.