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

Spec Conformance Review: Traces/Sampling #1596

Closed
5 tasks done
Aneurysm9 opened this issue Feb 25, 2021 · 6 comments · Fixed by open-telemetry/opentelemetry-specification#1523
Closed
5 tasks done
Assignees
Labels
area:trace Part of OpenTelemetry tracing pkg:SDK Related to an SDK package

Comments

@Aneurysm9
Copy link
Member

Aneurysm9 commented Feb 25, 2021

  • Review Sampler section of the specification and identify all normative requirements it contains.
  • Validate our implementation of the specification to conform or not. If it conforms, document the conformity here. If it does not open an Issue or PR track the work.
  • Review the Traces/Sampling section of the Spec Compliance Matrix.
  • If there are any items listed in matrix that are not required by the specification, need clarification, or are in conflict with the specification open a PR in the specification to correct it.
  • Update the matrix with the current state of Go (existing entries for Go should be ignored). If Go does not provide support for an item listed be sure to link to the Issue/PR tracking the work to support it.
@Aneurysm9 Aneurysm9 added the area:trace Part of OpenTelemetry tracing label Feb 25, 2021
@MrAlias MrAlias added the pkg:SDK Related to an SDK package label Mar 5, 2021
@MrAlias MrAlias self-assigned this Mar 9, 2021
@MrAlias
Copy link
Contributor

MrAlias commented Mar 9, 2021

Sampler

ShouldSample

Returns the sampling Decision for a Span to be created.

// Sampler decides whether a trace should be sampled and exported.
type Sampler interface {
ShouldSample(parameters SamplingParameters) SamplingResult
Description() string
}

Required arguments:

Arguments are passes as a SamplingParameter struct to not have an excessively long method signature and ensure future extensibility.

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

ParentContext trace.SpanContext

  • TraceId of the Span to be created.

TraceID trace.TraceID

If the parent SpanContext contains a valid TraceId, they MUST always match.

// TraceID already exists, just generate a SpanID
tid = parent.TraceID()

  • Name of the Span to be created.

  • SpanKind of the Span to be created.

Kind trace.SpanKind

  • Initial set of Attributes of the Span to be created.

Attributes []attribute.KeyValue

  • Collection of links that will be associated with the Span to be created.
    Typically useful for batch operations, see Links Between Spans.

Links []trace.Link

Return value:

Sampling results are returned as a SamplingResult struct.

It produces an output called SamplingResult which contains:

  • A sampling Decision. One of the following enum values:

Decision SamplingDecision

  • DROP - IsRecording() == false, span will not be recorded and all events and attributes will be dropped.
  • RECORD_ONLY - IsRecording() == true, but Sampled flag MUST NOT be set.
  • RECORD_AND_SAMPLE - IsRecording() == true AND Sampled flag MUST be set.

type SamplingDecision uint8
// Valid sampling decisions
const (
// Drop will not record the span and all attributes/events will be dropped
Drop SamplingDecision = iota
// Record indicates the span's `IsRecording() == true`, but `Sampled` flag
// *must not* be set
RecordOnly
// RecordAndSample has span's `IsRecording() == true` and `Sampled` flag
// *must* be set
RecordAndSample
)

  • A set of span Attributes that will also be added to the Span.

Attributes []attribute.KeyValue

The returned object must be immutable (multiple calls may return different immutable objects).

This is not a normative requirement based on the lack of capitalization of "must", but for what it's worth, we do not return attributes in any of the included Samplers.

  • A Tracestate that will be associated with the Span through the new SpanContext.

Tracestate trace.TraceState

If the sampler returns an empty Tracestate here, the Tracestate will be cleared, so samplers SHOULD normally return the passed-in Tracestate if they do not intend to change it.

Addressed in #1432

GetDescription

Description() string

Returns the sampler name or short description with the configuration.
This may be displayed on debug pages or in the logs.
Example: "TraceIdRatioBased{0.000100}".

Description MUST NOT change over time and caller can cache the returned value.

  • Ratio based sampler returns a non-exported field that is not changed after initialization:
    func (ts traceIDRatioSampler) Description() string {
    return ts.description
    }
  • Always on returns a static string:
    func (as alwaysOnSampler) Description() string {
    return "AlwaysOnSampler"
    }
  • Always off returns a static string:
    func (as alwaysOffSampler) Description() string {
    return "AlwaysOffSampler"
    }
  • Parent based returns a string comprised of non-exported fields that are not changed after initialization:
    func (pb parentBased) Description() string {
    return fmt.Sprintf("ParentBased{root:%s,remoteParentSampled:%s,"+
    "remoteParentNotSampled:%s,localParentSampled:%s,localParentNotSampled:%s}",
    pb.root.Description(),
    pb.config.remoteParentSampled.Description(),
    pb.config.remoteParentNotSampled.Description(),
    pb.config.localParentSampled.Description(),
    pb.config.localParentNotSampled.Description(),
    )
    }

@MrAlias
Copy link
Contributor

MrAlias commented Mar 9, 2021

Feature Go
Allow samplers to modify tracestate + Addressed in #1432
ShouldSample gets full parent Context +

@MrAlias
Copy link
Contributor

MrAlias commented Mar 9, 2021

I noticed that the Sampling section of the sdk spec is not covered by any other issue. Going to include it in the audit included in this issue.

@MrAlias
Copy link
Contributor

MrAlias commented Mar 9, 2021

Sampling

The OpenTelemetry API has two properties responsible for the data collection:

  • IsRecording field of a Span.
    If false the current Span discards all tracing data (attributes, events, status, etc.).
    Users can use this property to determine if collecting expensive trace data can be avoided.
    Span Processor MUST receive only those spans which have this field set to true.

If IsRecording returns false it is not sent to the span processors.

However, Span Exporter SHOULD NOT receive them unless the Sampled flag was also set.

  • Sampled flag in TraceFlags on SpanContext.
    This flag is propagated via the SpanContext to child Spans.
    For more details see the W3C Trace Context specification.
    This flag indicates that the Span has been sampled and will be exported.
    Span Exporters MUST receive those spans which have Sampled flag set to true and they SHOULD NOT receive the ones that do not.

This is a restatement of the above recommendation.

The flag combination SampledFlag == true and IsRecording == false could cause gaps in the distributed trace, and because of this OpenTelemetry API MUST NOT allow this combination.

Based on the way the sampling decision enums are structured in combination with the fact that isRecording is a superset of isSampled it is not possible for isSampled to return true without isRecording to also return true.

@MrAlias
Copy link
Contributor

MrAlias commented Mar 24, 2021

Required arguments:

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

ParentContext trace.SpanContext

This is not technically correct. A SpanContext is not a Span contained in a Context. I think this is going to ultimately be related to #1182 where we should probably be storing a span in the context, not a SpanContext.

@MrAlias
Copy link
Contributor

MrAlias commented Mar 24, 2021

Tracking here: #1727

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