-
Notifications
You must be signed in to change notification settings - Fork 710
Add sampler API, use in SDK tracer #225
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
Changes from all commits
496320d
21dd081
9ac4516
5e0370a
bc8782d
9a8f3c1
4264f16
567a7da
e743fdc
d90a8bc
7e0330b
a0978b2
7b1fccb
9b935ad
8de632d
644cf9d
6f0a33d
0de147e
c3efeb0
b24a465
235d74f
3594e32
1a4f96b
d6127b0
0e370ad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,15 @@ | ||
[flake8] | ||
ignore = E501,W503,E203 | ||
exclude = .svn,CVS,.bzr,.hg,.git,__pycache__,.tox,ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/gen/,ext/opentelemetry-ext-jaeger/build/* | ||
ignore = | ||
E501 # line too long, defer to black | ||
F401 # unused import, defer to pylint | ||
W503 # allow line breaks after binary ops, not after | ||
exclude = | ||
.bzr | ||
.git | ||
.hg | ||
.svn | ||
.tox | ||
CVS | ||
__pycache__ | ||
ext/opentelemetry-ext-jaeger/src/opentelemetry/ext/jaeger/gen/ | ||
ext/opentelemetry-ext-jaeger/build/* |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,125 @@ | ||
# Copyright 2019, OpenTelemetry Authors | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
import abc | ||
from typing import Dict, Mapping, Optional, Sequence | ||
|
||
# pylint: disable=unused-import | ||
from opentelemetry.trace import Link, SpanContext | ||
from opentelemetry.util.types import AttributeValue | ||
|
||
|
||
class Decision: | ||
"""A sampling decision as applied to a newly-created Span. | ||
|
||
Args: | ||
sampled: Whether the `Span` should be sampled. | ||
attributes: Attributes to add to the `Span`. | ||
""" | ||
|
||
def __repr__(self) -> str: | ||
return "{}({}, attributes={})".format( | ||
type(self).__name__, str(self.sampled), str(self.attributes) | ||
) | ||
|
||
def __init__( | ||
self, | ||
sampled: bool = False, | ||
attributes: Mapping[str, "AttributeValue"] = None, | ||
) -> None: | ||
self.sampled = sampled # type: bool | ||
if attributes is None: | ||
self.attributes = {} # type: Dict[str, "AttributeValue"] | ||
else: | ||
self.attributes = dict(attributes) | ||
|
||
|
||
class Sampler(abc.ABC): | ||
@abc.abstractmethod | ||
def should_sample( | ||
self, | ||
parent_context: Optional["SpanContext"], | ||
trace_id: int, | ||
span_id: int, | ||
name: str, | ||
links: Sequence["Link"] = (), | ||
) -> "Decision": | ||
pass | ||
|
||
|
||
class StaticSampler(Sampler): | ||
"""Sampler that always returns the same decision.""" | ||
|
||
def __init__(self, decision: "Decision"): | ||
self._decision = decision | ||
|
||
def should_sample( | ||
self, | ||
parent_context: Optional["SpanContext"], | ||
trace_id: int, | ||
span_id: int, | ||
name: str, | ||
links: Sequence["Link"] = (), | ||
) -> "Decision": | ||
return self._decision | ||
|
||
|
||
class ProbabilitySampler(Sampler): | ||
def __init__(self, rate: float): | ||
self._rate = rate | ||
self._bound = self.get_bound_for_rate(self._rate) | ||
|
||
# The sampler checks the last 8 bytes of the trace ID to decide whether to | ||
# sample a given trace. | ||
CHECK_BYTES = 0xFFFFFFFFFFFFFFFF | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the W3 spec, you should use the high ("left") 8 bytes instead:
But I wonder if we can find a more robust way to maybe randomly mix some bits here and there together. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I considered this but decided to stick to the OC behavior (or at least the intended behavior: this also fixes a rounding/OBO bug). FWIW checking the high bytes also seems more correct to me, but in practice -- if people are using short trace IDs in the wild -- sampling every request seems worse than sampling based on the non-random part. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you're right about this. I suggested it as a change to the spec in open-telemetry/opentelemetry-specification#331. Another fun benefit of checking bytes high-to-low is that the sampling decision should be mostly consistent between samplers that check different numbers of bytes. Unlike checking low-to-high where every incremental bit is effectively another coin toss. Ultimately we'll probably just put this number in the spec, but it's a neat property in any case. :D There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI - keep an eye on https://github.com/w3c/trace-context/pull/344/files. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's revisit this once open-telemetry/opentelemetry-specification#331 and w3c/trace-context#344 are resolved. |
||
|
||
@classmethod | ||
def get_bound_for_rate(cls, rate: float) -> int: | ||
return round(rate * (cls.CHECK_BYTES + 1)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If rate is less than There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually it is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I used
and etc. I think if you pass in a sampling rate This doesn't exactly work because float precision is worse than
Oh that's interesting, so the trace ID space is return 1 + round(rate * cls.CHECK_BYTES) I think that'd give us the same behavior as above, but always sample trace ID Should we have special handling for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm also happy to leave this as-is and treat There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have any preference here as I cannot think of a scenario where people would need such low but non-zero sample rate (unless someone is instrumenting SETI@home). 😆 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something I missed above: there are and |
||
|
||
@property | ||
def rate(self) -> float: | ||
return self._rate | ||
|
||
@rate.setter | ||
def rate(self, new_rate: float) -> None: | ||
self._rate = new_rate | ||
self._bound = self.get_bound_for_rate(self._rate) | ||
|
||
@property | ||
def bound(self) -> int: | ||
return self._bound | ||
|
||
def should_sample( | ||
self, | ||
parent_context: Optional["SpanContext"], | ||
trace_id: int, | ||
span_id: int, | ||
name: str, | ||
links: Sequence["Link"] = (), | ||
) -> "Decision": | ||
if parent_context is not None: | ||
return Decision(parent_context.trace_options.sampled) | ||
|
||
return Decision(trace_id & self.CHECK_BYTES < self.bound) | ||
|
||
|
||
# Samplers that ignore the parent sampling decision and never/always sample. | ||
ALWAYS_OFF = StaticSampler(Decision(False)) | ||
ALWAYS_ON = StaticSampler(Decision(True)) | ||
|
||
# Samplers that respect the parent sampling decision, but otherwise | ||
# never/always sample. | ||
DEFAULT_OFF = ProbabilitySampler(0.0) | ||
DEFAULT_ON = ProbabilitySampler(1.0) |
Uh oh!
There was an error while loading. Please reload this page.