add WithSpanKind option to span creation #234
Conversation
paivagustavo
requested review from
bogdandrutu,
jmacd,
pjanotti,
rghetia,
sjkaris and
tedsuo
as code owners
api/trace/api.go
Outdated
| @@ -137,13 +138,25 @@ type Link struct { | |||
| Attributes []core.KeyValue | |||
| } | |||
|
|
|||
| // SpanKind represents the role of a Span inside a Trace. Often, this defines how a Span | |||
| // will be processed and visualized by various backends. | |||
| type SpanKind int | |||
There was a problem hiding this comment.
I would prefer to make this type string. For the instances below, use the conventional name "server", "client", ...
I suggest this because in OpenTracing, instead of an explicit field in the span structure, a span.kind attribute was specified. In an OpenTracing bridge, we can just use SpanKind(value) and won't have to construct an explicit switch statement.
I think it's an added benefit that users can specify unconventional span kinds as well, although some will disagree. I see this value as an advisory statement, helpful to vendors, but we shouldn't force all span kinds to be one of these four values.
There was a problem hiding this comment.
I took the bullet here and changed it to string and implemented the bridge for SpanKind. I think this is fine for now, but we should go to specs to clarify the behavior of the SpanKind, java implementation is an Enum and only accept the values on the spec and any other value goes to internal.
WDYT?
There was a problem hiding this comment.
Thank you. 😄
I think we should do it, and I'll agree to back this up by filing an issue in the spec repo, sometime this week.
sdk/trace/trace_test.go
Outdated
| for _, sk := range sks { | ||
| var te testExporter | ||
| tp, _ := NewProvider(WithSyncer(&te), WithConfig(Config{DefaultSampler: AlwaysSample()})) | ||
| tr := tp.GetTracer("withSpanKind") |
There was a problem hiding this comment.
Why not reuse the exporter, provider and tracer from the outer scope?
There was a problem hiding this comment.
Previously, the endSpan method created a testExporter and used it to process a single span. But with the changes of the Provider, it started to receive the testExporter as a parameter, and it expected that we got exactly one span on the exporter, that why we should not reuse it.
There is a simpler way to do this, just reset the span list of the testExporter.
This updates the trace api with
SpanKindspec. Closes #233