Remove SpanData by bg451 · Pull Request #215 · open-telemetry/opentelemetry-specification

bg451 · 2019-08-14T20:15:53Z

Motivation

The goal of this change is to remove SpanData from the API specification and replace it by adding span start and end options. SpanData, from what I could see, had two primary use cases in mind. The first involved building out of band spans from different sources such as logs. The second was to allow deferred span creation, for example, receiving the first byte in a request, taking the time of that, parsing the headers, then finally creating the span with that first byte timestamp.

Changes

References to SpanData have been removed.

For span start options, we are adding an optional timestamp to specify the start time, a resource to override the tracer's resource, and span ID to support out of band reporting.

For span end options, we are adding an optional timestamp to specify the end time.

Maybe it's out of scope for this specification change and i can remove it, but I've also added an optional timestamp when adding events to support out of band events as well. See #213 for an example use case.

Original RFC discussion: open-telemetry/oteps#8
Merged RFC: https://github.com/open-telemetry/rfcs/blob/8dd6e21/text/0002-remove-spandata.md

bg451 · 2019-08-14T20:17:19Z

specification/api-tracing.md

+- `SpanID`
+- `Start timestamp`
+
+// TODO: There's a question about clock resolution when a user supplies a ts


This is an open question for me and I'm not quite sure what the solution is. I'd love to get your input and ideas @bogdandrutu and @tylerbenson

I think we should just say that when using an explicit start time, we recommend also using an explicit stop time, otherwise there is a chance that the clock readings are not comparable. If pressed, I would add that individual languages may decide to support >1 kind of timestamp where appropriate. E.g., In Python there is time.time() and time.time_ns() and both could be supported in some way.

Sounds like we should add End timestamp to the member list here as well.

I suspect I should just remove this todo altogether and just add a tidbit about languages recommending which clock should be used if supplying timestamps (assuming there's more than one way e.g. javascript and python). Does that seem like a reasonable solution?

bogdandrutu · 2019-08-14T21:32:02Z

I think we don't have the RFC approved, as per suggestion from @iredelmeier and @jmacd we merge all RFC with a proposed status and later we approve them (my initial thought was that once we merge the RFCs are approved or denied, but that is not the case) so please do a PR that changes the status to approved for that RFC before merging this.

I am not sure what is the process with RFC because I am not the one who proposed this process, so not clear for me what do we need to do to approve RFCs.

carlosalberto

Left a note about possibly adding End timestamp as part of this PR - that aside, it all looks great!

bogdandrutu · 2019-08-20T00:51:02Z

Please rebase. There are a lot of changes in this doc already merged :)

arminru · 2019-08-20T13:06:14Z

The RFC says:

From #71: If the underlying SDK automatically adds tags to spans such as thread-id, stacktrace, and cpu-usage when a span is started, they would be incorrect for out of band spans as the tracer would not know the difference between in and out of band spans. This can be mitigated by indicating that the span is out of band to prevent attaching incorrect information, possibly with an isOutOfBand() option on startSpan().

This flag/option is missing in this PR.

You noted above:

For span start options, we are adding an optional timestamp to specify the start time, a resource to override the tracer's resource, and span ID to support out of band reporting.

When spans are created asynchronously, e.g. from received messages or by reading a log file (see use cases described in #71), a Span ID will have to be generated at span creation and can therefore not be used to indicate that the span is reported out of band.

specification/api-tracing.md

bogdandrutu · 2019-09-04T18:05:22Z

Approvers please approve this PR. I think this is a trivial (and good) change applying the already approved RFC.

songy23

LGTM

bogdandrutu · 2019-09-06T15:56:03Z

specification/api-tracing.md

+
+- `Resource`
+- `SpanID`
+- `Start timestamp`


Start timestamp will not be removed :)

Oops will fix right now

No End timestamp btw? ;)

This is the section for span creation. The span end timestamp is mentioned here: https://github.com/open-telemetry/opentelemetry-specification/pull/215/files#diff-ea4f4a46fe8755cf03f9fb3a6434cb0cR373

songy23 · 2019-09-09T19:25:53Z

Please rebase.

bogdandrutu · 2019-09-09T20:16:57Z

Please follow what it says here https://github.com/open-telemetry/opentelemetry-specification/pull/215/checks?check_run_id=216754717

While I am trying to understand why DCO was added as a check please do this to not block this PR.

Signed-off-by: Brandon Gonzalez <bg@lightstep.com>

contest -> context Signed-off-by: Brandon Gonzalez <bg@lightstep.com>

Signed-off-by: Brandon Gonzalez <bg@lightstep.com>

…y#234) * Rename TraceOptions to TraceFlags to be w3c compatible Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com> * Fix more TraceOptions and rename recorded with sampled Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com> Signed-off-by: Brandon Gonzalez <bg@lightstep.com>

bg451 · 2019-09-09T20:22:34Z

Cool fixed DCO

yurishkuro · 2019-09-12T22:25:03Z

specification/api-tracing.md

 Start and end time as well as Event's timestamps MUST be recorded at a time of a
-calling of corresponding API and MUST not be passed as an argument. In order to
-record already completed span - [`SpanData`](#spandata) API HAVE TO be used.
+calling of corresponding API and MAY be used as an argument.


not sure what " and MAY be used as an argument" means. Is it needed here?

Hm yeah it doesn't read well, I don't think it's needed.

reyang

LGTM.

* Removes references of spandata Signed-off-by: Brandon Gonzalez <bg@lightstep.com> * initial stab at tracing spec Signed-off-by: Brandon Gonzalez <bg@lightstep.com> * adds link and event timestamp Signed-off-by: Brandon Gonzalez <bg@lightstep.com> * re-add whitespace to reduce diff size Signed-off-by: Brandon Gonzalez <bg@lightstep.com> * typo fix Signed-off-by: Brandon Gonzalez <bg@lightstep.com> * Adds OutOfBand flag to span options Signed-off-by: Brandon Gonzalez <bg@lightstep.com> * specification/api-distributedcontext: fix typo (open-telemetry#236) contest -> context Signed-off-by: Brandon Gonzalez <bg@lightstep.com> * Adds comment+link to Remove Out Of Band Support RFC Signed-off-by: Brandon Gonzalez <bg@lightstep.com> * remove comment in doc Signed-off-by: Brandon Gonzalez <bg@lightstep.com> * Remove work_in_progress dir (open-telemetry#225) Signed-off-by: Brandon Gonzalez <bg@lightstep.com> * Move start timestamp above the possibly removed fields Signed-off-by: Brandon Gonzalez <bg@lightstep.com> * Fix typo in FailedNotRetryable (open-telemetry#243) Signed-off-by: Brandon Gonzalez <bg@lightstep.com> * Rename TraceOptions to TraceFlags to be w3c compatible (open-telemetry#234) * Rename TraceOptions to TraceFlags to be w3c compatible Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com> * Fix more TraceOptions and rename recorded with sampled Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com> Signed-off-by: Brandon Gonzalez <bg@lightstep.com> * Respond to comment

Co-authored-by: jason plumb <75337021+breedx-splk@users.noreply.github.com> Co-authored-by: Chengzhong Wu <legendecas@gmail.com> Co-authored-by: Alexander Wert <AlexanderWert@users.noreply.github.com>

bg451 requested review from AloisReitbauer, SergeyKanzhelev, bogdandrutu, c24t, carlosalberto, iredelmeier, reyang, songy23, tedsuo and yurishkuro as code owners August 14, 2019 20:15

bg451 commented Aug 14, 2019

View reviewed changes

carlosalberto approved these changes Aug 19, 2019

View reviewed changes

fbogsany mentioned this pull request Aug 20, 2019

Update the span creation API open-telemetry/opentelemetry-ruby#53

Merged

bg451 force-pushed the bg/remove_spandata branch from a6ae58d to 55164f9 Compare August 23, 2019 20:45

bogdandrutu mentioned this pull request Aug 26, 2019

Add start time and duration (or end time) options on Span #139

Closed

bogdandrutu reviewed Aug 26, 2019

View reviewed changes

specification/api-tracing.md Show resolved Hide resolved

carlosalberto approved these changes Sep 4, 2019

View reviewed changes

bogdandrutu approved these changes Sep 4, 2019

View reviewed changes

specification/api-tracing.md Show resolved Hide resolved

songy23 approved these changes Sep 4, 2019

View reviewed changes

songy23 mentioned this pull request Sep 4, 2019

SDK: Reduce size of SpanData struct #77

Closed

tedsuo approved these changes Sep 6, 2019

View reviewed changes

bogdandrutu approved these changes Sep 6, 2019

View reviewed changes

Removes references of spandata

055aedc

Signed-off-by: Brandon Gonzalez <bg@lightstep.com>

Brandon Gonzalez and others added 12 commits September 9, 2019 16:19

initial stab at tracing spec

eb596c4

Signed-off-by: Brandon Gonzalez <bg@lightstep.com>

adds link and event timestamp

6bfe364

Signed-off-by: Brandon Gonzalez <bg@lightstep.com>

re-add whitespace to reduce diff size

9d41918

Signed-off-by: Brandon Gonzalez <bg@lightstep.com>

typo fix

5c8b003

Signed-off-by: Brandon Gonzalez <bg@lightstep.com>

Adds OutOfBand flag to span options

8e753a9

Signed-off-by: Brandon Gonzalez <bg@lightstep.com>

specification/api-distributedcontext: fix typo (open-telemetry#236)

86ca041

contest -> context Signed-off-by: Brandon Gonzalez <bg@lightstep.com>

Adds comment+link to Remove Out Of Band Support RFC

76c4ffa

Signed-off-by: Brandon Gonzalez <bg@lightstep.com>

remove comment in doc

e1be122

Signed-off-by: Brandon Gonzalez <bg@lightstep.com>

Remove work_in_progress dir (open-telemetry#225)

0a760c8

Signed-off-by: Brandon Gonzalez <bg@lightstep.com>

Move start timestamp above the possibly removed fields

18a36cd

Signed-off-by: Brandon Gonzalez <bg@lightstep.com>

Fix typo in FailedNotRetryable (open-telemetry#243)

41f08d3

Signed-off-by: Brandon Gonzalez <bg@lightstep.com>

bg451 force-pushed the bg/remove_spandata branch from ffe3441 to cda0350 Compare September 9, 2019 20:20

Merge branch 'master' into bg/remove_spandata

51c5c07

songy23 added the ready-to-merge label Sep 9, 2019

pavolloffay mentioned this pull request Sep 11, 2019

Add SDK span processors spec #205

Merged

yurishkuro approved these changes Sep 12, 2019

View reviewed changes

reyang approved these changes Sep 12, 2019

View reviewed changes

Respond to comment

74cedb2

yurishkuro merged commit 9d2edd2 into open-telemetry:master Sep 13, 2019

austinlparker mentioned this pull request Sep 17, 2019

Remove SpanData open-telemetry/opentelemetry-dotnet#207

Closed

This was referenced Sep 18, 2019

Add SpanData type open-telemetry/opentelemetry-js#72

Closed

chore: remove all references to SpanData open-telemetry/opentelemetry-js#286

Merged

jmacd mentioned this pull request Nov 10, 2020

Require the SDK to provide custom ID generation #1006

Merged

Conversation

bg451 commented Aug 14, 2019

Motivation

Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bogdandrutu commented Aug 14, 2019

Uh oh!

carlosalberto left a comment

Choose a reason for hiding this comment

Uh oh!

bogdandrutu commented Aug 20, 2019

Uh oh!

arminru commented Aug 20, 2019

Uh oh!

Uh oh!

Uh oh!

bogdandrutu commented Sep 4, 2019

Uh oh!

songy23 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

songy23 commented Sep 9, 2019

Uh oh!

bogdandrutu commented Sep 9, 2019

Uh oh!

bg451 commented Sep 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reyang left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants