Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generalize span creation #190

Merged
merged 18 commits into from
Aug 6, 2019
+172 −140
Merged

Generalize span creation #190

Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
ef2480e
Revise span creation, scrub references to builder
c24t Jul 22, 2019
6bd22e6
Make it clear that root spans aren't optional
c24t Jul 22, 2019
cae98f6
Move attrs, reword multiple sections
c24t Jul 23, 2019
e8d1dea
Lose StartSpan
c24t Jul 23, 2019
391857d
s/spans/span/
c24t Jul 23, 2019
a036be3
Hard wrap edits
c24t Jul 23, 2019
ed16890
Hard wrap rest of file
c24t Jul 23, 2019
7e9984f
Update active span description for PR comments
c24t Jul 23, 2019
2742bc7
Note that span attrs are ordered
c24t Jul 23, 2019
a4a8e38
Summarize span config, add details
c24t Jul 23, 2019
203a691
Apply @arminru's suggestions from code review
c24t Jul 24, 2019
3e6444a
Merge branch 'master' into span-creation-prose
c24t Jul 24, 2019
d2cebe9
Add notes on attribute, event, link ordering
c24t Jul 24, 2019
2e1af8e
Clarify optional/required span fields
c24t Jul 25, 2019
9271e1b
Merge branch 'master' into span-creation-prose
c24t Jul 25, 2019
0dc27c4
Remove attribute ordering note
c24t Jul 25, 2019
90e8366
Merge branch 'master' into span-creation-prose
yurishkuro Jul 30, 2019
d397b38
Merge branch 'master' into span-creation-prose
carlosalberto Aug 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Summarize span config, add details
@c24t
c24t committed Jul 23, 2019

Verified

This commit was created on github.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
commit a4a8e3821dafbb4e686d2e36f094153d88492bff
17 changes: 13 additions & 4 deletions specification/tracing-api.md
View file
Original file line number Diff line number Diff line change
@@ -233,13 +233,15 @@ sub-operations.
`Span`s encapsulate:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great overview! 👍


- The operation name
- A [`SpanContext`](#SpanContext)
- A parent [`Span`](#Span) or [`SpanContext`](#SpanContext)
- An immutable [`SpanContext`](#SpanContext) that uniquely identifies the
`Span`
- A parent span in the form of a [`Span`](#Span), [`SpanContext`](#SpanContext),
or null
- A start timestamp
- An end timestamp
- An ordered mapping of [`Attribute`s](#SetAttribute)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

- A list of [`Link`s](#AddLink)
- A list of [`Event`s](#AddEvent)
- A list of [`Link`s](#AddLink) to other `Span`s
- A list of timestamped [`Event`s](#AddEvent)

The `Span`'s start and end timestamps reflect the elapsed real time of the
operation. A `Span`'s start time SHOULD be set to the current time on [span
@@ -261,6 +263,13 @@ 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 API MUST allow users to provide the following properties:
- The operation name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this new?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


The `Tracer` MUST allow the caller to specify the new `Span`'s parent in the
form of a `Span` or `SpanContext`. The `Tracer` SHOULD create each new `Span` as
a child of its active `Span` unless an explicit parent is provided.