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

Add requirements for how the Trace API should behave in the absence of an SDK #719

Merged

Conversation

jkwatson
Copy link
Contributor

@jkwatson jkwatson commented Jul 20, 2020

Clarifies the behavior of the Trace API, in the absence of an installed SDK, especially with respect to context propagation.

Fixes #689

Changes

Adds requirements for Trace API behavior in the absence of an SDK.

requested parent SpanContext:

* A valid SpanContext is specified as the parent of the new Span: The API MUST treat this parent context as the
context for the newly created Span. This means that a SpanContext that has been provided by a configured Propagator
Copy link
Member

Choose a reason for hiding this comment

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

Bit confused here.. This section is talking about the scenario when there is no Sdk installed. In that scenario, should we still take and propagate the context? Is this propagation limited to in-proc spans only or out-of-proc as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, yes, that's the point of this PR. :) Since you can install a Propagator without an SDK installed, it became important to explain how that would work. If a propagator (let's say, the W3C Trace Context propagator) is installed, this specifies that that API should still propagate incoming spans, transparently.

It's the "minimum" requirement from the W3C spec for forwarding a trace: https://www.w3.org/TR/trace-context/#design-overview

Copy link
Member

Choose a reason for hiding this comment

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

It's the "minimum" requirement from the W3C spec for forwarding a trace

If you have no API and no SDK you are not a participant in the trace and thus not accountable to minimum behaviors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But, if you have propagators attached to the API, shouldn't you be?

@dyladan
Copy link
Member

dyladan commented Jul 20, 2020

This is not possible to implement in javascript without moving a ton of in-process context propagation logic into the API. Why is it that one use-case gets special treatment of being enabled by default?

@jkwatson
Copy link
Contributor Author

@dyladan can you comment in the originating issue, please? #689

I thought this had been fairly well agreed to by all who had participated there.

@dyladan
Copy link
Member

dyladan commented Jul 20, 2020

@dyladan can you comment in the originating issue, please? #689

I thought this had been fairly well agreed to by all who had participated there.

I don't see that agreement. Can you point to a specific comment? I also looked through the associated #428 and saw only no-op there in the absence of the SDK.

@jkwatson
Copy link
Contributor Author

@dyladan can you comment in the originating issue, please? #689
I thought this had been fairly well agreed to by all who had participated there.

I don't see that agreement. Can you point to a specific comment? I also looked through the associated #428 and saw only no-op there in the absence of the SDK.

Well, that entire issue is about the behavior of the API with no SDK, so yes, it seems like general agreement was come to, and I agreed to own writing up the results of the discussion.

@bogdandrutu bogdandrutu added area:api Cross language API specification issue spec:trace Related to the specification/trace directory labels Jul 21, 2020
specification/trace/api.md Outdated Show resolved Hide resolved
specification/trace/api.md Outdated Show resolved Hide resolved

In general, in the absence of an installed SDK, the Trace API is intended to a "no-op" API.
This means that operations on a Tracer, or on Spans, should have no side effects and do nothing. However, there
is one important exception to this general rule, and that is related to propagation of a SpanContext.
Copy link
Contributor

Choose a reason for hiding this comment

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

This part will probably change, based on #721

Copy link
Member

Choose a reason for hiding this comment

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

You should be able to install a W3C propagator into the propagation layer (which is supposed to be an artifact separate from API) even without initializing the SDK, right? So I don't think this part changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I don't think this changes. IF there is a propagator installed in the propagation APIs, THEN, this requires the API to propagate any valid spans that are specified as parent spans, even if no real child-span is created.

@carlosalberto
Copy link
Contributor

Thanks for the effort - I'm wondering if this should also be added to Metrics/CorrelationContext parts as well.

@bogdandrutu
Copy link
Member

@jkwatson please rebase and either remove draft or close this PR, we provided enough "initial" feedback :)

@jkwatson
Copy link
Contributor Author

I'll update this and get it ready for official review/merging.

@jkwatson jkwatson marked this pull request as ready for review August 10, 2020 20:53
@jkwatson jkwatson requested review from a team August 10, 2020 20:53
@bogdandrutu
Copy link
Member

Rebase again :)

@jkwatson jkwatson requested review from a team and yurishkuro August 12, 2020 20:41
@jkwatson
Copy link
Contributor Author

Rebase again :)

done :)

@bogdandrutu
Copy link
Member

I think there is an issue with GitHub status checks

@bogdandrutu bogdandrutu reopened this Aug 13, 2020
@tedsuo
Copy link
Contributor

tedsuo commented Aug 17, 2020

This looks ready to go. Can we merge?

@bogdandrutu
Copy link
Member

@carlosalberto as assigned member please merge or comment :)

@bogdandrutu
Copy link
Member

Merging this since it is non controversial. @jkwatson we should have similar content for all the APIs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Specify the behavior of the Tracer APIs in the absence of an SDK
9 participants