RFC for Removing and replacing SpanData by bg451 · Pull Request #8 · open-telemetry/oteps

bg451 · 2019-07-12T21:15:10Z

No description provided.

0004-remove-spandata.md

reyang · 2019-07-12T21:39:25Z

0004-remove-spandata.md

+
+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.


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.

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 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.

bogdandrutu

Overall LG with some minor comments. But I am happy to approve this if they are solved.

0004-remove-spandata.md

bogdandrutu · 2019-07-15T23:29:28Z

0004-remove-spandata.md

+
+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.


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.

0004-remove-spandata.md

tsloughter · 2019-07-17T15:02:33Z

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.

bg451 · 2019-07-17T21:22:28Z

as saying there is no way to add to a span during its life and only when it starts or finishes?

@tsloughter This wouldn't be the case, just talking about the concept of bulk logging when a span finishes, which is something opentracing allows.

tsloughter · 2019-07-17T21:33:00Z

@bg451 ah, ok. The phrasing, "getting rid of SpanData and tracer.recordSpanData() and replacing it by allowing tracer.startSpan() and span.finish() to accept start and end timestamp options", threw me off.

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.

bg451 · 2019-07-17T21:52:41Z

Is this also around being able to create spans after the fact from log data?

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

tsloughter · 2019-07-17T21:57:33Z

@bg451 yup, exactly that, thanks.

bogdandrutu · 2019-07-18T08:12:49Z

This is currently blocked on the author side to respond to all the comments. Waiting for that before another review.

0004-remove-spandata.md

bogdandrutu · 2019-08-06T15:03:15Z

@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.

0004-remove-spandata.md

pavolloffay · 2019-08-08T09:16:55Z

There are benefits of defining it in the SDK. For instance sdk-processors spec can reference it.

bogdandrutu · 2019-08-08T13:33:02Z

@bg451 please fix the last two comments and I will merge this.

bg451 · 2019-08-08T16:45:59Z

There are benefits of defining it in the SDK.

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.

text/0002-remove-spandata.md

bogdandrutu · 2019-08-09T00:15:50Z

@bg451 please also respond to my comments in this post #8 (comment)

…move_spandata

bg451 · 2019-08-09T18:19:10Z

Oh huh I thought I pushed my changes yesterday morn, looks like I didn't.

@bogdandrutu 375c397#diff-f62a07a10e742d00e2ef6471e1d80d13R39 re your comment

bogdandrutu · 2019-08-09T19:03:59Z

@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.

c24t

LGTM

* RFC: Remove SpanData * formatting * respond to comments, add related issues * finishing touches * rename file * respond to comments * edit withOption wording * Update 0004-remove-spandata.md Co-Authored-By: Yang Song <songy23@users.noreply.github.com> * fix typo/github links * Adds language for including span ID, removes it from open questions * respond to comments * Rename 0004-remove-spandata.md to text/0002-remove-spandata.md * Change finish to end * Clarify where the isOutOfBand option passed.

Brandon Gonzalez added 5 commits July 9, 2019 00:31

RFC: Remove SpanData

89326d3

formatting

2370a29

respond to comments, add related issues

06a7d4a

finishing touches

17296c8

rename file

c836b85

bg451 requested review from AloisReitbauer, SergeyKanzhelev, bogdandrutu, c24t, carlosalberto, iredelmeier, reyang, songy23, tedsuo and yurishkuro as code owners July 12, 2019 21:15

reyang reviewed Jul 12, 2019

View reviewed changes

0004-remove-spandata.md Outdated Show resolved Hide resolved

reyang reviewed Jul 12, 2019

View reviewed changes

bogdandrutu reviewed Jul 15, 2019

View reviewed changes

bogdandrutu assigned bg451 Jul 18, 2019

respond to comments

687eb90

bogdandrutu approved these changes Jul 22, 2019

View reviewed changes

0004-remove-spandata.md Outdated Show resolved Hide resolved

Merge branch 'master' into bg/remove_spandata

9b7c290

tylerbenson reviewed Jul 23, 2019

View reviewed changes

0004-remove-spandata.md Outdated Show resolved Hide resolved

songy23 reviewed Jul 23, 2019

View reviewed changes

0004-remove-spandata.md Outdated Show resolved Hide resolved

0004-remove-spandata.md Outdated Show resolved Hide resolved

c24t mentioned this pull request Aug 7, 2019

Add SDK span processors spec open-telemetry/opentelemetry-specification#205

Merged

pavolloffay reviewed Aug 8, 2019

View reviewed changes

0004-remove-spandata.md Outdated Show resolved Hide resolved

Brandon Gonzalez and others added 3 commits August 8, 2019 12:54

respond to comments

375c397

Rename 0004-remove-spandata.md to text/0002-remove-spandata.md

c482a59

Change finish to end

7b8d345

bogdandrutu reviewed Aug 9, 2019

View reviewed changes

text/0002-remove-spandata.md Outdated Show resolved Hide resolved

Merge branch 'bg/remove_spandata' of github.com:bg451/rfcs into bg/re…

377e3b6

…move_spandata

Clarify where the isOutOfBand option passed.

1659dc0

carlosalberto approved these changes Aug 9, 2019

View reviewed changes

c24t approved these changes Aug 9, 2019

View reviewed changes

bogdandrutu merged commit 8625418 into open-telemetry:master Aug 9, 2019

bg451 mentioned this pull request Aug 14, 2019

Remove SpanData open-telemetry/opentelemetry-specification#215

Merged


		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.

Conversation

bg451 commented Jul 12, 2019

Uh oh!

Uh oh!

reyang Jul 12, 2019

Choose a reason for hiding this comment

Uh oh!

bogdandrutu Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

jmacd Jul 16, 2019

Choose a reason for hiding this comment

Uh oh!

bogdandrutu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bogdandrutu Jul 15, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tsloughter commented Jul 17, 2019

Uh oh!

bg451 commented Jul 17, 2019

Uh oh!

tsloughter commented Jul 17, 2019

Uh oh!

bg451 commented Jul 17, 2019

Uh oh!

tsloughter commented Jul 17, 2019

Uh oh!

bogdandrutu commented Jul 18, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bogdandrutu commented Aug 6, 2019

Uh oh!

Uh oh!

pavolloffay commented Aug 8, 2019

Uh oh!

bogdandrutu commented Aug 8, 2019

Uh oh!

bg451 commented Aug 8, 2019

Uh oh!

Uh oh!

bogdandrutu commented Aug 9, 2019

Uh oh!

bg451 commented Aug 9, 2019

Uh oh!

bogdandrutu commented Aug 9, 2019

Uh oh!

c24t left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants