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.

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

Change Span specification to focus more on capabilities. #184

Merged
merged 16 commits into from
Jul 24, 2019

Conversation

bogdandrutu
Copy link
Member

This PR changes a subset of the Span API to the agreed model in #165. The rest of the changes will be added in a different PR, trying to keep this PR small so feedback can be addressed faster.

Updates #165

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

API MUST also provide an overload that accepts a [`Link` interface](#link). This
overload allows instrumentation to supply a lazily calculated `Link`.
#### Set Status
Copy link
Member

Choose a reason for hiding this comment

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

Is the span status supposed to be any custom user-defined string, or will an enum or constants be defined?

Copy link
Member Author

@bogdandrutu bogdandrutu Jul 18, 2019

Choose a reason for hiding this comment

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

I did not get to the Status API. I will clarify in the followup PR.


Retrieve the `Span`s `SpanContext`
A `Span` MUST have the ability to return the `SpanContext` associated with it.
The returned value MUST be the same for the entire Span lifetime.
Copy link
Member

Choose a reason for hiding this comment

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

Isn't sampled flag a part of span context? In some implementations it could change during the lifetime of a span. What is the actual requirement here?

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 the current PR does not change the behavior. I do understand your requirement but we should have a separate discussion about this. Filed #189 to track this separately.

specification/tracing-api.md Outdated Show resolved Hide resolved
specification/tracing-api.md Outdated Show resolved Hide resolved
specification/tracing-api.md Outdated Show resolved Hide resolved
- An API to record lazy initialized links. This can be implemented by providing
a `Link` interface or a concrete `Link` definition and a `LinkFormatter`. If
the language supports overloads then this SHOULD be called `AddLink` otherwise
`AddLazyLink` may be consider.
Copy link
Member

Choose a reason for hiding this comment

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

is there a ticket describing the use case for "lazy links"?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is mostly for the attributes, for example the values may be lazily formatted.

Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

LGTM

specification/tracing-api.md Show resolved Hide resolved
specification/tracing-api.md Outdated Show resolved Hide resolved
specification/tracing-api.md Outdated Show resolved Hide resolved
specification/tracing-api.md Outdated Show resolved Hide resolved
specification/tracing-api.md Outdated Show resolved Hide resolved
specification/tracing-api.md Outdated Show resolved Hide resolved
@tsloughter
Copy link
Member

This reads much better :).

I was writing up a whole thing about how in Erlang, with immutable variables, SpanContext maps fine to how the API is defined but the Span's themselves don't. The actual spans are stored in a mutable table keyed on the spancontext. so calls like set_status take the span context and update the status in the table without ever reading the whole span out.. But this PR seems to resolve all that and fits much better.

Co-Authored-By: Chris Kleinknecht <[email protected]>
@bogdandrutu
Copy link
Member Author

Waiting for other approves before I will merge.

@bogdandrutu
Copy link
Member Author

I will wait until tomorrow to merge this, if no comments :)

@bogdandrutu bogdandrutu merged commit a69ac84 into open-telemetry:master Jul 24, 2019
@bogdandrutu bogdandrutu deleted the spancap branch July 24, 2019 12:43
SergeyKanzhelev pushed a commit to SergeyKanzhelev/opentelemetry-specification that referenced this pull request Feb 18, 2020
…try#184)

* Change Span specification to focus more on capabilities.

* Update specification/tracing-api.md

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

* Update specification/tracing-api.md

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

* Update specification/tracing-api.md

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

* Update specification/tracing-api.md

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

* Update specification/tracing-api.md

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

* Update specification/tracing-api.md

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

* Update specification/tracing-api.md

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

* Update specification/tracing-api.md

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

* Update specification/tracing-api.md

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

* Update specification/tracing-api.md

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

* Update specification/tracing-api.md

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

* Update specification/tracing-api.md

Co-Authored-By: Chris Kleinknecht <[email protected]>

* Update specification/tracing-api.md

Co-Authored-By: Chris Kleinknecht <[email protected]>

* Fix comments from review.

* Clarify that only one attribute,event,link is recorded.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants