Generalize span creation #190

c24t · 2019-07-23T00:09:42Z

Updates #165.

This PR removes references to the java-specific SpanBuilder in the span creation spec, and borrows from the java documentation and OpenTelemetry spec.

@songy23, @bogdandrutu I'd like to get your feedback here before moving on to span operations.

specification/tracing-api.md

reyang

LGTM.

bogdandrutu · 2019-07-23T10:13:49Z

specification/tracing-api.md

-latency and, optionally, one or more sub-spans for its sub-operations.
+A `Span` represents a single operation within a trace.
+Spans can be nested to form a trace tree.
+Each trace contains a root span, which typically describes the end-to-end latency and, optionally, one or more sub-spans for its sub-operations.


we should use a limit for every line. I think the current vscode config asks for 80.

Changed in a036be3 and ed16890.

We're not doing this consistently across the repo, e.g. #186 uses a single line for each paragraph.

I think there's a good argument for soft-wrapping sentences or paragraphs, especially since putting each sentence on its own line means most edits show up as line diffs instead of spilling over into other lines.

I'm pretty surprised to see a vscode config file in this repo.

specification/tracing-api.md

bogdandrutu · 2019-07-23T10:24:03Z

specification/tracing-api.md


- Name of the span.
+### Span Creation


I would summarize all the configs that we want users to be able to provide, currently it is hard to find this list because every option is introduced in one paragraph.

### Span Creation Implementations MUST provide a way to create `Span`s via a `Tracer`, which is responsible for tracking the currently active `Span` and MAY provide default options for newly created `Span`s. The users of this API SHOULD be able to provide the following properties: * A parent Span (using a Span, SpanContext, or no parent). * Parent links. * ...

Changed in a4a8e38 and reworded to make it less redundant with the list above, let me know if this is what you have in mind.

specification/tracing-api.md

songy23

LGTM

arminru

LGTM, thanks!

specification/tracing-api.md

arminru · 2019-07-24T12:25:28Z

specification/tracing-api.md

+the end-to-end latency and, optionally, one or more sub-spans for its
+sub-operations.
+
+`Span`s encapsulate:


great overview! 👍

specification/tracing-api.md

bogdandrutu · 2019-07-24T12:42:00Z

specification/tracing-api.md

+- The parent span, and whether the new `Span` should be a root `Span`.
+- `Attribute`s
+- `Link`s
+- `Event`s


Is this new?

It's the items from the list above that are set by the user. I added this in response to #190 (comment).

bogdandrutu · 2019-07-24T12:42:41Z

specification/tracing-api.md

+options for newly created `Span`s.
+
+The API MUST allow users to provide the following properties:
+- The operation name


This should be marked as required, the others are optional (specify the default).

How about this?

The API SHOULD require the caller to provide: - The operation name - The parent span, and whether the new `Span` should be a root `Span`. The API MUST allow users to provide the following properties, which SHOULD be empty by default: - `Attribute`s - `Link`s - `Event`s

reyang · 2019-07-24T17:24:21Z

specification/tracing-api.md

+  or null
+- A start timestamp
+- An end timestamp
+- An ordered mapping of [`Attribute`s](#SetAttribute)


Do we want to enforce ordering in the spec?

I understand that the actual implementation might put a limit on the maximum number of attributes, and in order to achieve this, the implementation could choose ordered mapping to implement FIFO. But is this a scenario/semantic we want to mention in the spec or it is just implementation details?

Ordering won't be reflected directly in the APIs but it still affects the overall behavior. I think it's worth mentioning in the spec so that all implementation can conform to it.

Let's say, if there is an implementation which supports unlimited number of attributes, do we still want it to maintain the order? What would the consumer do with the ordering?

If we do have ordering requirement, it might be worthy to mention implementation which puts limit on the number of key-value-pairs should follow FIFO.
Another detail that we might want to cover, if there is a duplicated key but different value, what's the intended behavior. For links/events, if the user is trying to append a value which already exists, should the library de-dupe, report error, or append?

if there is an implementation which supports unlimited number of attributes, do we still want it to maintain the order?

If we do have ordering requirement, it might be worthy to mention implementation which puts limit on the number of key-value-pairs should follow FIFO.

Agreed if there's no limit specified for attributes, rephrase it to FIFO may be clearer. However I think preserving the order for timed events makes more sense. To simplify things I would recommend we keep the same for events, links and attributes.

For links/events, if the user is trying to append a value which already exists, should the library de-dupe, report error, or append?

I would say the library would just append instead of de-dup. For events specifically, it's difficult to determine if two events are duplicated since events depend on the time when they are added.

Would like to hear others' opinions on this.

I think it makes sense to preserve ordering, especially since it may be exporters instead of the span impl that's responsible for truncating these. In that case we'd use the ordering even if there's no limit in the span.

I would say the library would just append instead of de-dup

I'd expect attributes tobe de-duped since (this version of) the spec says they're stored in a map, events and links get appended to the list.

Also "ordering" here doesn't specify anything about the order, just that it's deterministic.

Ah that's correct, for attributes we should do de-dup.

Regarding de-dupe on the order mapping, we also need to mention the ordering (the latest value of the same key would win, and the key-value pair will be treated as the newest inserted one). We could borrow idea from TraceContext spec.

I added a note on de-duping under "Set Attributes" in d2cebe9. I'll edit the span operations section next and expand on this there.

songy23

Please rebase.

Co-Authored-By: Armin Ruech <[email protected]>

c24t · 2019-07-26T21:55:09Z

This should be ready to merge.

carlosalberto · 2019-08-06T14:34:44Z

Merging now, as this has been approved for a few days now. We can always receive feedback later on ;)

@arminru

* Revise span creation, scrub references to builder * Make it clear that root spans aren't optional * Move attrs, reword multiple sections * Lose StartSpan * Update active span description for PR comments * Note that span attrs are ordered * Summarize span config, add details * Apply @arminru's suggestions from code review Co-Authored-By: Armin Ruech <[email protected]> * Add notes on attribute, event, link ordering * Clarify optional/required span fields * Remove attribute ordering note

@arminru

* Revise span creation, scrub references to builder * Make it clear that root spans aren't optional * Move attrs, reword multiple sections * Lose StartSpan * Update active span description for PR comments * Note that span attrs are ordered * Summarize span config, add details * Apply @arminru's suggestions from code review Co-Authored-By: Armin Ruech <[email protected]> * Add notes on attribute, event, link ordering * Clarify optional/required span fields * Remove attribute ordering note

c24t added 4 commits July 22, 2019 16:17

Revise span creation, scrub references to builder

ef2480e

Make it clear that root spans aren't optional

6bd22e6

Move attrs, reword multiple sections

cae98f6

Lose StartSpan

e8d1dea

c24t requested a review from songy23 July 23, 2019 00:09

c24t requested review from AloisReitbauer, bogdandrutu, carlosalberto, iredelmeier, reyang, SergeyKanzhelev, tedsuo and yurishkuro as code owners July 23, 2019 00:09

reyang reviewed Jul 23, 2019

View reviewed changes

specification/tracing-api.md Outdated Show resolved Hide resolved

reyang approved these changes Jul 23, 2019

View reviewed changes

s/spans/span/

391857d

bogdandrutu reviewed Jul 23, 2019

View reviewed changes

songy23 reviewed Jul 23, 2019

View reviewed changes

specification/tracing-api.md Outdated Show resolved Hide resolved

c24t added 4 commits July 23, 2019 12:43

Hard wrap edits

a036be3

Hard wrap rest of file

ed16890

Update active span description for PR comments

7e9984f

Note that span attrs are ordered

2742bc7

songy23 approved these changes Jul 23, 2019

View reviewed changes

Summarize span config, add details

a4a8e38

c24t mentioned this pull request Jul 23, 2019

Settle on linebreak style #192

Closed

arminru approved these changes Jul 24, 2019

View reviewed changes

bogdandrutu reviewed Jul 24, 2019

View reviewed changes

reyang reviewed Jul 24, 2019

View reviewed changes

songy23 reviewed Jul 24, 2019

View reviewed changes

Apply @arminru's suggestions from code review

203a691

Co-Authored-By: Armin Ruech <[email protected]>

c24t added 5 commits July 24, 2019 11:58

Merge branch 'master' into span-creation-prose

3e6444a

Add notes on attribute, event, link ordering

d2cebe9

Clarify optional/required span fields

2e1af8e

Merge branch 'master' into span-creation-prose

9271e1b

Remove attribute ordering note

0dc27c4

Merge branch 'master' into span-creation-prose

90e8366

tedsuo approved these changes Jul 30, 2019

View reviewed changes

tedsuo added the ready-to-merge label Jul 30, 2019

Merge branch 'master' into span-creation-prose

d397b38

carlosalberto merged commit 25d90b1 into open-telemetry:master Aug 6, 2019

c24t mentioned this pull request Aug 9, 2019

Generalize span operations #209

Merged

c24t removed the ready-to-merge label Aug 9, 2019

c24t deleted the span-creation-prose branch August 15, 2019 20:30

TuckTuckFloof pushed a commit to TuckTuckFloof/opentelemetry-specification that referenced this pull request Oct 15, 2020

Add JSON C++ support - Bazel (open-telemetry#190)

2c8b0e3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize span creation #190

Generalize span creation #190

c24t commented Jul 23, 2019 •

edited by bogdandrutu

Loading

reyang left a comment

bogdandrutu Jul 23, 2019

c24t Jul 23, 2019

c24t Jul 23, 2019

bogdandrutu Jul 23, 2019

c24t Jul 23, 2019

songy23 left a comment

arminru left a comment

arminru Jul 24, 2019

bogdandrutu Jul 24, 2019

c24t Jul 24, 2019

bogdandrutu Jul 24, 2019

c24t Jul 24, 2019

reyang Jul 24, 2019

songy23 Jul 24, 2019

reyang Jul 24, 2019

songy23 Jul 24, 2019

c24t Jul 24, 2019

songy23 Jul 24, 2019

reyang Jul 24, 2019

c24t Jul 24, 2019

songy23 left a comment

c24t commented Jul 26, 2019

carlosalberto commented Aug 6, 2019

Generalize span creation #190

Generalize span creation #190

Conversation

c24t commented Jul 23, 2019 • edited by bogdandrutu Loading

reyang 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 left a comment

Choose a reason for hiding this comment

arminru 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

songy23 left a comment

Choose a reason for hiding this comment

c24t commented Jul 26, 2019

carlosalberto commented Aug 6, 2019

c24t commented Jul 23, 2019 •

edited by bogdandrutu

Loading