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 should_sample parameters to be spec compliant #1764

Merged
merged 9 commits into from
Apr 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Added `py.typed` file to every package. This should resolve a bunch of mypy
errors for users.
([#1720](https://github.com/open-telemetry/opentelemetry-python/pull/1720))
- Added `SpanKind` to `should_sample` parameters, suggest using parent span context's tracestate
instead of manually passed in tracestate in `should_sample`
([#1764](https://github.com/open-telemetry/opentelemetry-python/pull/1764))

### Changed
- Adjust `B3Format` propagator to be spec compliant by not modifying context
Expand Down
7 changes: 2 additions & 5 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -905,20 +905,17 @@ def start_span( # pylint: disable=too-many-locals
if parent_span_context is None or not parent_span_context.is_valid:
parent_span_context = None
trace_id = self.id_generator.generate_trace_id()
trace_flags = None
trace_state = None
else:
trace_id = parent_span_context.trace_id
trace_flags = parent_span_context.trace_flags
trace_state = parent_span_context.trace_state

# The sampler decides whether to create a real or no-op span at the
# time of span creation. No-op spans do not record events, and are not
# exported.
# The sampler may also add attributes to the newly-created span, e.g.
# to include information about the sampling result.
# The sampler may also modify the parent span context's tracestate
sampling_result = self.sampler.should_sample(
context, trace_id, name, attributes, links, trace_state
context, trace_id, name, kind, attributes, links
)

trace_flags = (
Expand Down
28 changes: 24 additions & 4 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/sampling.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@

Custom samplers can be created by subclassing `Sampler` and implementing `Sampler.should_sample` as well as `Sampler.get_description`.

Samplers are able to modify the `opentelemetry.trace.span.TraceState` of the parent of the span being created. For custom samplers, it is suggested to implement `Sampler.should_sample` to utilize the
parent span context's `opentelemetry.trace.span.TraceState` and pass into the `SamplingResult` instead of the explicit trace_state field passed into the parameter of `Sampler.should_sample`.

To use a sampler, pass it into the tracer provider constructor. For example:

.. code:: python
Expand Down Expand Up @@ -108,7 +111,7 @@
OTEL_TRACES_SAMPLER,
OTEL_TRACES_SAMPLER_ARG,
)
from opentelemetry.trace import Link, get_current_span
from opentelemetry.trace import Link, SpanKind, get_current_span
from opentelemetry.trace.span import TraceState
from opentelemetry.util.types import Attributes

Expand Down Expand Up @@ -167,6 +170,7 @@ def should_sample(
parent_context: Optional["Context"],
trace_id: int,
name: str,
kind: SpanKind = None,
attributes: Attributes = None,
links: Sequence["Link"] = None,
trace_state: "TraceState" = None,
Expand All @@ -189,13 +193,18 @@ def should_sample(
parent_context: Optional["Context"],
trace_id: int,
name: str,
kind: SpanKind = None,
attributes: Attributes = None,
links: Sequence["Link"] = None,
trace_state: "TraceState" = None,
) -> "SamplingResult":
if self._decision is Decision.DROP:
attributes = None
return SamplingResult(self._decision, attributes, trace_state)
return SamplingResult(
self._decision,
attributes,
_get_parent_trace_state(parent_context),
)

def get_description(self) -> str:
if self._decision is Decision.DROP:
Expand Down Expand Up @@ -246,6 +255,7 @@ def should_sample(
parent_context: Optional["Context"],
trace_id: int,
name: str,
kind: SpanKind = None,
attributes: Attributes = None,
links: Sequence["Link"] = None,
trace_state: "TraceState" = None,
Expand All @@ -255,7 +265,9 @@ def should_sample(
decision = Decision.RECORD_AND_SAMPLE
if decision is Decision.DROP:
attributes = None
return SamplingResult(decision, attributes, trace_state)
return SamplingResult(
decision, attributes, _get_parent_trace_state(parent_context),
)

def get_description(self) -> str:
return "TraceIdRatioBased{{{}}}".format(self._rate)
Expand Down Expand Up @@ -296,6 +308,7 @@ def should_sample(
parent_context: Optional["Context"],
trace_id: int,
name: str,
kind: SpanKind = None,
attributes: Attributes = None,
links: Sequence["Link"] = None,
trace_state: "TraceState" = None,
Expand All @@ -322,9 +335,9 @@ def should_sample(
parent_context=parent_context,
trace_id=trace_id,
name=name,
kind=kind,
attributes=attributes,
links=links,
trace_state=trace_state,
)

def get_description(self):
Expand Down Expand Up @@ -385,3 +398,10 @@ def _get_from_env_or_default() -> Sampler:
return _KNOWN_SAMPLERS[trace_sampler](rate)

return _KNOWN_SAMPLERS[trace_sampler]


def _get_parent_trace_state(parent_context) -> Optional["TraceState"]:
parent_span_context = get_current_span(parent_context).get_span_context()
if parent_span_context is None or not parent_span_context.is_valid:
return None
return parent_span_context.trace_state
Loading