Remove SpanData #215

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 <[email protected]>

contest -> context Signed-off-by: Brandon Gonzalez <[email protected]>

Signed-off-by: Brandon Gonzalez <[email protected]>

…y#234) * Rename TraceOptions to TraceFlags to be w3c compatible Signed-off-by: Bogdan Drutu <[email protected]> * Fix more TraceOptions and rename recorded with sampled Signed-off-by: Bogdan Drutu <[email protected]> Signed-off-by: Brandon Gonzalez <[email protected]>

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

Cool fixed DCO

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

specification/api-tracing.md

@@ -388,8 +381,7 @@ timestamps to the Span object:
 - The end time needs to be recorded when the operation is ended.

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

bg451 requested review from AloisReitbauer, bogdandrutu, c24t, carlosalberto, iredelmeier, reyang, SergeyKanzhelev, 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 <[email protected]>

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

initial stab at tracing spec

eb596c4

Signed-off-by: Brandon Gonzalez <[email protected]>

adds link and event timestamp

6bfe364

Signed-off-by: Brandon Gonzalez <[email protected]>

re-add whitespace to reduce diff size

9d41918

Signed-off-by: Brandon Gonzalez <[email protected]>

typo fix

5c8b003

Signed-off-by: Brandon Gonzalez <[email protected]>

Adds OutOfBand flag to span options

8e753a9

Signed-off-by: Brandon Gonzalez <[email protected]>

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

86ca041

contest -> context Signed-off-by: Brandon Gonzalez <[email protected]>

Adds comment+link to Remove Out Of Band Support RFC

76c4ffa

Signed-off-by: Brandon Gonzalez <[email protected]>

remove comment in doc

e1be122

Signed-off-by: Brandon Gonzalez <[email protected]>

Remove work_in_progress dir (open-telemetry#225)

0a760c8

Signed-off-by: Brandon Gonzalez <[email protected]>

Move start timestamp above the possibly removed fields

18a36cd

Signed-off-by: Brandon Gonzalez <[email protected]>

Fix typo in FailedNotRetryable (open-telemetry#243)

41f08d3

Signed-off-by: Brandon Gonzalez <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove SpanData #215

Remove SpanData #215

bg451 commented Aug 14, 2019

bg451 Aug 14, 2019

jmacd Aug 14, 2019

carlosalberto Aug 19, 2019

bg451 Aug 27, 2019

bogdandrutu commented Aug 14, 2019

carlosalberto left a comment

bogdandrutu commented Aug 20, 2019

arminru commented Aug 20, 2019

bogdandrutu commented Sep 4, 2019

songy23 left a comment

bogdandrutu Sep 6, 2019

bg451 Sep 6, 2019

bg451 Sep 6, 2019

carlosalberto Sep 6, 2019

Oberon00 Sep 9, 2019

songy23 commented Sep 9, 2019

bogdandrutu commented Sep 9, 2019

bg451 commented Sep 9, 2019

yurishkuro Sep 12, 2019

bg451 Sep 13, 2019

reyang left a comment

Remove SpanData #215

Remove SpanData #215

Conversation

bg451 commented Aug 14, 2019

Motivation

Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdandrutu commented Aug 14, 2019

carlosalberto left a comment

Choose a reason for hiding this comment

bogdandrutu commented Aug 20, 2019

arminru commented Aug 20, 2019

bogdandrutu commented Sep 4, 2019

songy23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

songy23 commented Sep 9, 2019

bogdandrutu commented Sep 9, 2019

bg451 commented Sep 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyang left a comment

Choose a reason for hiding this comment