Prototype to show how Typed Spans can be implemented #964
Conversation
thisthat
requested review from
arminru,
bogdandrutu,
carlosalberto,
jkwatson,
pavolloffay,
songy23 and
tylerbenson
as code owners
...ed_span/src/main/java/io/opentelemetry/sdk/contrib/typedspan/http/HttpClientSpanWrapper.java
Show resolved
Hide resolved
...ed_span/src/main/java/io/opentelemetry/sdk/contrib/typedspan/http/HttpClientSpanWrapper.java
Show resolved
Hide resolved
...ed_span/src/main/java/io/opentelemetry/sdk/contrib/typedspan/http/HttpClientSpanWrapper.java
Show resolved
Hide resolved
...ed_span/src/main/java/io/opentelemetry/sdk/contrib/typedspan/http/HttpClientSpanWrapper.java
Show resolved
Hide resolved
...d_span/src/main/java/io/opentelemetry/sdk/contrib/typedspan/http/HttpSpanWrapperBuilder.java
Show resolved
Hide resolved
| * Semantic Attributes | ||
| * https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/data-http.md | ||
| */ | ||
| public static final String METHOD_KEY = "http.method"; |
There was a problem hiding this comment.
This should later be switched to use #758.
There was a problem hiding this comment.
This is just a prototype to showcase how the API should look like. It is not meant to be merged
...rib/typed_span/src/main/java/io/opentelemetry/sdk/contrib/typedspan/http/HttpStatusCode.java
Show resolved
Hide resolved
| /** Set up status code to "reason phrase" map. */ | ||
| static { | ||
| // HTTP 1.0 Server status codes -- see RFC 1945 | ||
| addStatusCodeMap(SC_OK, "OK"); |
There was a problem hiding this comment.
I don't think we need to add the status description. The status_text is optional and I think it should be left out if it adds no information beyond the status code integer.
|
Hi @thisthat! Any chance you can join the auto-instr-java SIG meeting next Thu at 9am Pacific Time and we can discuss the two typed spans proposals? I think the auto-instrumentation project would be a good place to incubate this idea, and once we prove it out (apply it to all of the auto-instrumentation), we can promote it to this repo. |
|
@trask sure! Sounds a good idea! |
|
I'd strongly prefer this to be in the main opentelemetry-java repository in a contrib package, even if auto-instr is among the first users. |
| import io.opentelemetry.trace.Span; | ||
|
|
||
| public abstract class BaseSpanWrapper { | ||
| Span span; |
There was a problem hiding this comment.
It doesn't feel like this is pulling much weight, and maybe just adding extra mental overhead. Do you imagine more things going in here?
There was a problem hiding this comment.
We might insert some hook mechanism for the auto-instrumentation people.
There was a problem hiding this comment.
After seeing open-telemetry/opentelemetry-java-instrumentation#195, I think what the auto-instrumentation SIG calls typed spans is actually unrelated to this PR, so I wonder if we will ever need any hooks.
|
Note that there is concept also currently called "typed spans" in the auto-instrumentation repository, but apart from the name it seems to be mostly unrelated: open-telemetry/opentelemetry-java-instrumentation#195 |
|
Does this need to be kept open, or has it fulfilled its purpose? |
This prototype addresses the discussion of #778