Skip to content

Commit 96cd202

Browse files
authored
Sampler to default to parentbased_always_on if OTEL_TRACES_SAMPLER is Unset (#140)
*Issue #, if available:* When `OTEL_TRACES_SAMPLER` is unset, the following error occurs when applying auto-instrumentation: ``` File "/Users/test/Documents/my_test_venv/lib/python3.12/site-packages/amazon/opentelemetry/distro/aws_opentelemetry_configurator.py", line 136, in _init_tracing sampler = _customize_sampler(sampler) ^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/test/Documents/my_test_venv/lib/python3.12/site-packages/amazon/opentelemetry/distro/aws_opentelemetry_configurator.py", line 210, in _customize_sampler return AlwaysRecordSampler(sampler) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/test/Documents/my_test_venv/lib/python3.12/site-packages/amazon/opentelemetry/distro/always_record_sampler.py", line 37, in __init__ raise ValueError("root_sampler must not be None") ``` If `OTEL_TRACES_SAMPLER` is not set, this error will occur because we don’t fallback to a default sampler In upstream OTel Python, this is fine, because the following events occur: ``` OTEL_TRACES_SAMPLER is unset -> _get_sampler returns None TracerProvider receives NONE sampler -> TracerProvider defaults to `parentbased_always_on` ``` In this repository, this is not fine because the following events occur: ``` OTEL_TRACES_SAMPLER is unset -> _get_sampler returns None _get_sampler returns None -> _custom_import_sampler returns None _customize_sampler receives None sampler -> AlwaysRecordSampler receives NONE sampler and raises error ``` *Description of changes:* Currently, `_get_sampler()` method returns `environ.get(OTEL_TRACES_SAMPLER, None)`. However, OTel Docs states that `OTEL_TRACES_SAMPLER` should [default to parentbased_always_on](https://opentelemetry.io/docs/languages/sdk-configuration/general/#otel_traces_sampler). Thus the change is to default to `parentbased_always_on` if _get_sampler() returns None. This solution was chosen as it requires the least amount of changes from upstream. - An alternative solution is to replace `_import_sampler()` method with `_get_from_env_or_default()`, which would read from `OTEL_TRACES_SAMPLER` env var a second time. Ideally, upstream's `_get_sampler()` should default to `parentbased_always_on` *Testing:* 1. Before change, `unset OTEL_TRACES_SAMPLER`, see ValueError when instrumenting `server_automatic_s3client.py` 2. After change, `unset OTEL_TRACES_SAMPLER`, no more error occurred. Traces from `server_automatic_s3client.py` were recorded in XRay console. Also tested with `OTEL_TRACES_SAMPLER=xray` to ensure xray sampler still works. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
1 parent d0ab51a commit 96cd202

File tree

2 files changed

+17
-1
lines changed

2 files changed

+17
-1
lines changed

aws-opentelemetry-distro/src/amazon/opentelemetry/distro/aws_opentelemetry_configurator.py

+8
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,14 @@ def _exclude_urls_for_instrumentations():
169169

170170

171171
def _custom_import_sampler(sampler_name: str, resource: Resource) -> Sampler:
172+
# sampler_name from _get_sampler() can be None if `OTEL_TRACES_SAMPLER` is unset. Upstream TracerProvider is able to
173+
# accept None as sampler, however we require the sampler to be not None to create `AlwaysRecordSampler` beforehand.
174+
# Default value of `OTEL_TRACES_SAMPLER` should be `parentbased_always_on`.
175+
# https://opentelemetry.io/docs/languages/sdk-configuration/general/#otel_traces_sampler
176+
# Ideally, _get_sampler() should default to `parentbased_always_on` in upstream.
177+
if sampler_name is None:
178+
sampler_name = "parentbased_always_on"
179+
172180
if sampler_name == "xray":
173181
# Example env var value
174182
# OTEL_TRACES_SAMPLER_ARG=endpoint=http://localhost:2000,polling_interval=360

aws-opentelemetry-distro/tests/amazon/opentelemetry/distro/test_aws_opentelementry_configurator.py

+9-1
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
from opentelemetry.sdk.resources import Resource
2626
from opentelemetry.sdk.trace import Span, SpanProcessor, Tracer, TracerProvider
2727
from opentelemetry.sdk.trace.export import SpanExporter
28-
from opentelemetry.sdk.trace.sampling import Sampler
28+
from opentelemetry.sdk.trace.sampling import DEFAULT_ON, Sampler
2929
from opentelemetry.trace import get_tracer_provider
3030

3131

@@ -153,6 +153,14 @@ def test_import_xray_sampler_with_invalid_environment_arguments(self):
153153
xray_client._AwsXRaySamplingClient__get_sampling_rules_endpoint, "http://127.0.0.1:2000/GetSamplingRules"
154154
)
155155

156+
def test_import_default_sampler_when_env_var_is_not_set(self):
157+
os.environ.pop(OTEL_TRACES_SAMPLER, None)
158+
default_sampler: Sampler = _custom_import_sampler(None, resource=None)
159+
160+
self.assertIsNotNone(default_sampler)
161+
self.assertEqual(default_sampler.get_description(), DEFAULT_ON.get_description())
162+
# DEFAULT_ON is a ParentBased(ALWAYS_ON) sampler
163+
156164
def test_using_xray_sampler_sets_url_exclusion_env_vars(self):
157165
targets_to_exclude = "SamplingTargets,GetSamplingRules"
158166
os.environ.pop("OTEL_PYTHON_REQUESTS_EXCLUDED_URLS", None)

0 commit comments

Comments
 (0)