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

Sampling parameters require a Context with an embedded Span #1727

Closed
MrAlias opened this issue Mar 24, 2021 · 2 comments · Fixed by #1749
Closed

Sampling parameters require a Context with an embedded Span #1727

MrAlias opened this issue Mar 24, 2021 · 2 comments · Fixed by #1749
Assignees
Labels
area:trace Part of OpenTelemetry tracing pkg:SDK Related to an SDK package
Milestone

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Mar 24, 2021

Currently we use the following sampling decision parameteres:

// SamplingParameters contains the values passed to a Sampler.
type SamplingParameters struct {
ParentContext trace.SpanContext
TraceID trace.TraceID
Name string
HasRemoteParent bool
Kind trace.SpanKind
Attributes []attribute.KeyValue
Links []trace.Link
}

The specification requires:

  • Context with parent Span.
    The Span's SpanContext may be invalid to indicate a root span.
    ...

This likely needs to be addressed more holistically here first.

@MrAlias MrAlias added area:trace Part of OpenTelemetry tracing pkg:SDK Related to an SDK package release:1.0.0-rc.1 labels Mar 24, 2021
@MrAlias MrAlias added this to the RC1 milestone Mar 24, 2021
@Aneurysm9
Copy link
Member

I think this also ties in to the concept of a "propagation span" that only has a span context and is non-recording. I think with the IsRemote predicate on span contexts we're now closer to being able to simply stick a span into a context with only a remote span context and let the SDK figure out what to do with it when starting a new span.

@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 25, 2021

I think this also ties in to the concept of a "propagation span" that only has a span context and is non-recording. I think with the IsRemote predicate on span contexts we're now closer to being able to simply stick a span into a context with only a remote span context and let the SDK figure out what to do with it when starting a new span.

Agreed! That is a change I am working on submitting here (which will resolve "Reconsider the context keys"). It requires this to merge first, but I think we can achieve this now.

MrAlias added a commit to MrAlias/opentelemetry-go that referenced this issue Mar 29, 2021
Remove HasRemoteParent fields from SamplingParameters. The
HasRemoteParent field is a duplicate of the Remote field of the parent
span context contained in the ParentContext.

Change the `ParentContext` field from storing a `SpanContext` to a
`context.Context` that holds the parent span. This is to conform with
the OpenTelemetry specification and resolve open-telemetry#1727.
@MrAlias MrAlias self-assigned this Mar 29, 2021
MrAlias added a commit that referenced this issue Mar 30, 2021
* Update SamplingParameters

Remove HasRemoteParent fields from SamplingParameters. The
HasRemoteParent field is a duplicate of the Remote field of the parent
span context contained in the ParentContext.

Change the `ParentContext` field from storing a `SpanContext` to a
`context.Context` that holds the parent span. This is to conform with
the OpenTelemetry specification and resolve #1727.

* Update PR number
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:trace Part of OpenTelemetry tracing pkg:SDK Related to an SDK package
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants