RFC for Removing and replacing SpanData #8
Conversation
bg451
requested review from
AloisReitbauer,
bogdandrutu,
c24t,
carlosalberto,
iredelmeier,
reyang,
SergeyKanzhelev,
songy23,
tedsuo and
yurishkuro
as code owners
0004-remove-spandata.md
Outdated
|
|
||
| SpanData's primary use case comes from the need to construct and report out of band spans, meaning that you're creating "custom" spans for an operation you don't own. A good example of this is a program that takes in structured logs that contain correlation IDs and a duration (e.g. from splunk) and [converts them](https://github.com/lightstep/splunktospan/blob/master/splunktospan/span.py#L43) to spans for your tracing system. [Another example](https://github.com/lightstep/haproxy_log2span/blob/master/lib/lib.go#L292) is running a sidecar on an HAProxy machine, tailing the request logs, and creating spans. SpanData supports the out of band reporting case, whereas you can’t with the current Span API as you cannot set the start and end timestamp, or add any tags at span creation that the sampler might need. | ||
|
|
||
| I'd like to propose getting rid of SpanData and `tracer.recordSpanData()` and replacing it by allowing `tracer.startSpan()` and `span.finish()` to accept start and end timestamp options. This reduces the API surface, consolidating on a single span type. Options would meet the requirements for out of band reporting. |
There was a problem hiding this comment.
Maybe it was my misunderstanding, I thought SpanData was targeting consumers (e.g. exporters) where immutable is desired.
Span was targeting producers, where different components can grab the "current active span" from the context/tracer and append extra information.
There was a problem hiding this comment.
SpanData in the API has the motivation explained by the RFC. In the SDK may have a different usage, but if we need something in the SDK then we can define it there.
There was a problem hiding this comment.
There has been additional confusion because the exporters declare a SpanData type, but it's not the same thing that's been discussed as a way to handle "out of band" spans. This RFC is about removing SpanData from the API, not the observer.
0004-remove-spandata.md
Outdated
|
|
||
| SpanData's primary use case comes from the need to construct and report out of band spans, meaning that you're creating "custom" spans for an operation you don't own. A good example of this is a program that takes in structured logs that contain correlation IDs and a duration (e.g. from splunk) and [converts them](https://github.com/lightstep/splunktospan/blob/master/splunktospan/span.py#L43) to spans for your tracing system. [Another example](https://github.com/lightstep/haproxy_log2span/blob/master/lib/lib.go#L292) is running a sidecar on an HAProxy machine, tailing the request logs, and creating spans. SpanData supports the out of band reporting case, whereas you can’t with the current Span API as you cannot set the start and end timestamp, or add any tags at span creation that the sampler might need. | ||
|
|
||
| I'd like to propose getting rid of SpanData and `tracer.recordSpanData()` and replacing it by allowing `tracer.startSpan()` and `span.finish()` to accept start and end timestamp options. This reduces the API surface, consolidating on a single span type. Options would meet the requirements for out of band reporting. |
There was a problem hiding this comment.
SpanData in the API has the motivation explained by the RFC. In the SDK may have a different usage, but if we need something in the SDK then we can define it there.
|
I reread this and now I'm back to reading it as saying there is no way to add to a span during its life and only when it starts or finishes? If that is the case one worry I have is exception handling. If you can only add events/logs/status when starting or finishing a span it removes the possibility to add those attributes during the life of the span and finishing the span in the handling of a thrown exception. |
@tsloughter This wouldn't be the case, just talking about the concept of bulk logging when a span finishes, which is something opentracing allows. |
|
@bg451 ah, ok. The phrasing, "getting rid of SpanData and Is this also around being able to create spans after the fact from log data? This is important to me for support of an Erlang/Elixir library that outputs events like database query times after the db call that we'd like to generate spans from and attach as children to the process that makes the db request. |
If I'm understanding the question correctly, yes this is one of the supported use cases. If you can extract a trace context/identifier from your log, you'd be able to make it a child span in that trace. For example (with opentracing start options), this is a sidecar program that would tail HAProxy logs, look for a span context to become a child of, then create/finish the span with the timestamp and tags coming from the parsed log line: https://github.com/lightstep/haproxy_log2span/blob/master/lib/lib.go#L290 |
|
@bg451 yup, exactly that, thanks. |
|
This is currently blocked on the author side to respond to all the comments. Waiting for that before another review. |
|
Hey hey @bogdandrutu I will merge this PR later today unless you have something to say ;) And of course, you can always provide feedback/PRs later on. |
|
@carlosalberto one small issue that can be resolved in the specification is time correlation (ensuring monotonic time is used). We do want to ensure inside a span or a sub-trace (spans generated by the same process) that the timestamp is correctly (monotonic) calculated. I would suggest just a small item in the future work for the moment. |
|
There are benefits of defining it in the SDK. For instance sdk-processors spec can reference it. |
|
@bg451 please fix the last two comments and I will merge this. |
Just to clarify, it being spandata here right? If so I agree, there still needs to be some sort of concrete type that SDK exporters can read from. |
|
@bg451 please also respond to my comments in this post #8 (comment) |
|
Oh huh I thought I pushed my changes yesterday morn, looks like I didn't. @bogdandrutu 375c397#diff-f62a07a10e742d00e2ef6471e1d80d13R39 re your comment |
|
@tedsuo @carlosalberto @AloisReitbauer @yurishkuro @SergeyKanzhelev @c24t any more comments on this? I would like to merge it but I am looking to have at least one or ideally two more approvers. |
No description provided.