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

Make tracer to add ignored parents as links as an option, add initial semantic conventions for links #436

Closed
wants to merge 2 commits into from

Conversation

krnowak
Copy link
Member

@krnowak krnowak commented Feb 4, 2020

In opentelemetry-go, we have implemented (but not yet merged - open-telemetry/opentelemetry-go#451) an option for forcing the tracer to create a new root span (this is required by the specification). We decided that putting the possible-but-ignored parents as link into the span can be useful. This is not currently mandated by the specification, so this PR tries to take a stab at it. I'm not sure if the place where I propose the attribute keys and values for link attributes is the right place, so please correct me if it is not. Thanks.

@Oberon00
Copy link
Member

Oberon00 commented Feb 4, 2020

@krnowak

(this is required by the specification)

Which specification? W3C or OTel? Can you please add a link?

EDIT: Also, the go PR you linked is huge. Can you please summarize the relevant parts here so that one can understand the issue without reading the PR diff?

@dyladan
Copy link
Member

dyladan commented Feb 4, 2020

As I understand it, when they call startSpan, it implicitly gets a parent from the current context. They added an option to force it to become a root span.

link to spec for that: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-tracing.md#span-creation

The API MUST accept the following parameters:

  • The parent Span or a Context containing a parent Span or SpanContext, and whether the new Span should be a root Span.

This PR makes it so that when you call startSpan and force it to be a root span in the case where there was a Span or SpanContext that would have become the parent automatically, that Span/SpanContext which would have been the parent (but isn't because this is a root span) should be added as a Link.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

I think this is the first time we have a specific prescription for link attributes. I am not sure this doc is the place for it, or the only place, because this is clearly a semantic convention that should be documented in the data-* docs. And if we have more conventions for link attributes, I would like them to be consolidated in one document.

On the subject of capturing ignored parent, it sounds like a reasonable behavior.

@krnowak
Copy link
Member Author

krnowak commented Feb 4, 2020

@Oberon00

About requirement - I was referring to the sentence "Implementations MUST provide an option to create a Span as a root span, and MUST generate a new TraceId for each root span created.".

And the relevant changes in the PR are here:

@krnowak krnowak force-pushed the krnowak/withnewroot branch from 3893f14 to 7fd4608 Compare February 4, 2020 20:00
@krnowak
Copy link
Member Author

krnowak commented Feb 4, 2020

Updated the PR. Added a new document with semantic conventions for link attributes.

@krnowak krnowak force-pushed the krnowak/withnewroot branch from 7fd4608 to 8f54aca Compare February 5, 2020 08:52
@krnowak krnowak changed the title Require tracer to add ignored parents as links Require tracer to add ignored parents as links, add initial semantic conventions for links Feb 5, 2020
@krnowak krnowak force-pushed the krnowak/withnewroot branch from 8f54aca to e3589f3 Compare February 5, 2020 16:51
@krnowak krnowak changed the title Require tracer to add ignored parents as links, add initial semantic conventions for links Make tracer to add ignored parents as links as an option, add initial semantic conventions for links Feb 5, 2020
@krnowak
Copy link
Member Author

krnowak commented Feb 5, 2020

Updated.

specification/data-link-semantic-conventions.md Outdated Show resolved Hide resolved

| Attribute | Description | Example | Required? |
|---|---|---|---|
| ignored-parent.kind | Kind of ignored span context. It MUST be either "local" or "remote". "local" denotes a span context that was a part of a current span at the time of a creation of the link. "remote" denotes a span context that was a part of remote span at the time of a creation of the link. | "local" | Yes |
Copy link
Member

Choose a reason for hiding this comment

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

That would be the first semantic conventions to use dashes in keys. I suggest using ignoredparent or ignored_parent instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to keep the word "ignored" out. That will be clear in context. Maybe instead just parent.kind=PSK where PSK is the parent SpanKind.

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'd go with the "local" and "remote" kinds for now, because getting the parent SpanKind would require an access to the Span, which might not be available if all we got is the span context from the propagator.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the prefix to parent.

This also adds an initial document about semantic conventions of the
link attributes.
Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

I agree with @yurishkuro that adding the link types should be a bigger discussion. It also feels that the proposed flag addresses a very specific use case and there may be more requirements. Can the same be implemented via SpanProcessor.Start callback so more elaborate logic can be added if needed?

@@ -286,6 +286,14 @@ a root span, and MUST generate a new `TraceId` for each root span created.
For a Span with a parent, the `TraceId` MUST be the same as the parent.
Also, the child span MUST inherit all `TraceState` values of its parent by default.

The option for creating a span as a root span should allow to choose whether the
Copy link
Member

Choose a reason for hiding this comment

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

another option may be to add traceparent of the ignored parent to the tracestate. It may be more involved design as we will need to decide how the key in tracestate will be called and how to write the original traceparent value there.

What scenario do you envision here? Can it be implemented via the SpanProcessor so customer may apply some custom logic like I described with the tracestate?

@carlosalberto carlosalberto added spec:trace Related to the specification/trace directory area:semantic-conventions Related to semantic conventions labels Jun 12, 2020
@carlosalberto carlosalberto added the release:after-ga Not required before GA release, and not going to work on before GA label Jul 2, 2020
@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Aug 12, 2020
@github-actions
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Aug 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions release:after-ga Not required before GA release, and not going to work on before GA spec:trace Related to the specification/trace directory Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants