Use OS entropy in RandomIdGenerator so random.seed() cannot repeat trace/span IDs - #5590
Use OS entropy in RandomIdGenerator so random.seed() cannot repeat trace/span IDs#5590geekflyer wants to merge 1 commit into
Conversation
|
|
RandomIdGenerator drew IDs from the process-global random module, so applications that call random.seed() for reproducibility (common in ML workloads) made separate processes and restarts generate identical trace and span IDs, silently corrupting or overwriting traces. Draw IDs from secrets.randbits() (OS entropy) instead: unaffected by random.seed(), and not copied across fork(). Decouple the statistical composite-sampler test from the conftest random_seed fixture by seeding a local generator that reproduces the identical ID stream. Fixes open-telemetry#4376
04ae8f3 to
8bcbfea
Compare
Pull request dashboard statusWaiting on reviewers · refreshed 2026-08-28 21:09 UTC Review the latest changes. Status above doesn't look right?
|
|
The changes in this PR can have impact on performance, please take a look here. |
|
If I understand this correctly: There is seeding mechanism A which is currently being used in the SDK. Using mechanism A in the SDK causes problems because there are many applications that also use A. But what about the applications that use mechanism B? Would that introduce the same problem as using mechanism A? I understand that maybe it is impossible not to affect any application so that we have to choose between A and B and B can be the one who affects applications less. |
|
Seeding is something that we need for testing. Every test run must be seeded so that we can get reproducible results. I think the changes in this PR will break this. I am ok with using a better seeding mechanism if that is advantageous but we gotta find the way of keeping our tests also seeded. |
I'm aware, but I think they're acceptable, especially considering that other SDKs like Go and co. go through the same path |
the issue with mechanism A is that it can be set to a fixed seed (shared state if you will) that leads to ID conflicts when doing any sort of multi-threading. Mechanism B doesn't have a way to set a fixed seed, so no matter how many applications use mechanism B, they won't interfere which each other. |
The PR already addresses this. https://github.com/open-telemetry/opentelemetry-python/pull/5590/changes#diff-09cc59b650f6838fc3236c1ad85d8af81ea8b0a23a428a5f9ad095345d7ff6e4R64 injects a fixed seed random generator instead of using the default one. |
What I mean is that for our tests we need to do seeding at one single place and that should do the seeding that makes every single test in the run reproducible. For example, having that seeding mechanism in a I have not checked yet, we may not have that mechanism in place and if not this is a different issue. |
Hmm, if mechanism B does not have a way to set a fixed seed, how do we set the fixed seed we need for testing? 🤔 |
I don't think that mechanism is in place yet and currently nothing other than conftest.py relies on seeded randomness, otherwise my change would've already broken many tests. |
well, to be more precise, the new code makes otel-python use by default a randomness source that CANNOT be altered by anyone, including otel-python itself or other unrelated application code. However one can still pass another id generator that uses a different (deterministic) randomness source to TracerProvider, which is basically what you'd do in tests. |
Description
Fixes #4376
RandomIdGenerator— the default ID generator everyTracerProvideruses — draws trace and span IDs from the process-globalrandommodule:Any application that calls
random.seed(...)therefore fixes the exact sequence of trace and span IDs the SDK will generate. Seeding the global generator is standard practice for reproducibility in machine-learning and scientific workloads, and those applications get identical trace and span IDs across process restarts and across replicas — separate runs silently overwrite or corrupt each other's traces in the backend.This PR makes
RandomIdGeneratordraw IDs from operating-system entropy viasecrets.randbits()instead. OS entropy cannot be seeded from Python and is not copied acrossfork(), so seeding can no longer make separate processes deterministically repeat IDs — and pre-forking servers (e.g. Gunicorn) no longer have workers that inherit identical generator state.Reproduction
Before this change, every process running this prints the same "random" ID:
which is byte-for-byte the duplicate trace ID reported in #4376. Two eval jobs, two service replicas, or one service before/after a restart all emit spans under the same trace ID.
How other implementations handle this
The other major OTel SDKs are already immune to application seeding, and the most prominent downstream Python consumer has shipped a workaround for this SDK:
math/rand/v2top-level functions (sdk/trace/id_generator.goat v1.46.0, L30–L69).math/rand/v2auto-seeds its global generator from OS entropy and removedrand.Seedentirely, so applications cannot influence the stream (Gomath/rand/v2docs). Even before the v2 migration in open-telemetry/opentelemetry-go#6732, the generator was never application-seedable: it seeded a private source fromcrypto/randat construction (id_generator.goat ffe855df33, L75–L81).RandomIdGeneratordraws fromRandomSupplier.platformDefault()(RandomIdGenerator.javaat v1.65.0, L18), which resolves toThreadLocalRandom::current(RandomSupplier.javaat v1.65.0, L27–L33).ThreadLocalRandom.setSeedthrowsUnsupportedOperationException(JDK docs), so applications cannot seed it.MLFLOW_TRACE_USE_ISOLATED_RANDOM_ID_GENERATOR(environment_variables.py@ 948322ac, L1006–L1008) with an_IsolatedRandomIdGeneratorwhose docstring names this exact defect: "Unlike the default OTelRandomIdGenerator, this is immune torandom.seed()calls in user code, preventing duplicate trace/span IDs across re-runs" (mlflow/tracing/provider.py@ 948322ac, L90–L118).Why
secretsand not a privaterandom.Random()A private
random.Random()instance (as proposed in #4377) fixes therandom.seed()case but keeps two failure modes:fork()duplication — a Mersenne Twister instance created beforefork()(preforking servers,multiprocessingwith the fork start method) is copied byte-for-byte into every child, and all children then generate the same ID sequence.MLflow's workaround above is the working proof of this trade-off: because it uses a private
random.Random(), it must also maintain a registry of every generator instance and re-seed them all viaos.register_at_fork(after_in_child=...)to avoid forked children inheriting identical state (mlflow/tracing/provider.py@ 948322ac, L76–L87).secrets.randbits()reads from the OS CSPRNG (os.urandom), which is seedless and stateless per-read, so both problems disappear with a one-line change and no fork hooks. It also keepsis_trace_id_random()honest: previously the SDK set the W3Crandom-trace-idflag while emitting fully deterministic IDs under a seeded generator. All 128/64 bits remain uniformly random, asTraceIdRatioBased-style samplers require.Performance: measured with
timeiton CPython 3.12 (Linux x86_64):secrets.randbits(64)≈ 546 ns/call vsrandom.getrandbits(64)≈ 59 ns/call, andsecrets.randbits(128)≈ 549 ns — about 0.5 µs extra per generated ID (onegetrandom(2)syscall). Span creation performs sampling, attribute handling, context operations, and clock reads that cost tens of µs in this SDK; at 10 000 spans/s the added cost is ~10 ms/s of one core.Changes
opentelemetry-sdk/src/opentelemetry/sdk/trace/id_generator.pyrandom.getrandbits→secrets.randbits; docstring documents the entropy source and fork behavioropentelemetry-sdk/tests/trace/test_trace.pyopentelemetry.sdk.trace.id_generator.secrets.randbits(mock-where-used) and assert exact call lists; new regression testtest_ids_unaffected_by_global_random_seedopentelemetry-sdk/tests/trace/composite_sampler/test_traceid_ratio.pytest_samplinggenerates trace IDs from a locally seededrandom.Random(0)instead ofRandomIdGenerator.changelog/<PR>.fixedWhy the sampler test change is needed
opentelemetry-sdk/tests/conftest.pyinstalls an autouse fixture that callsrandom.seed(0)before every test — the SDK's own test suite relied on the seedable ID generator to make its statistical assertions deterministic.test_samplingasserts a sampled count within ±50 of the expectation over 10 000 IDs; at ratio 0.45 that tolerance is about one standard deviation, so with genuinely random IDs the test would fail roughly 30% of the time. Seeding a localrandom.Random(0)for the test's ID stream keeps it deterministic, and matches the ID stream the test consumed before this change (the globalrandomfunctions delegate to a hiddenRandominstance with identical seeding). The test exercises the sampler, not the ID generator, so coverage is unchanged.Behavior change note
If an application relied on
random.seed()to obtain reproducible trace/span IDs (snapshot tests, demos), that stops working — deliberately, since IDs are wire-level identifiers whose uniqueness is what backends key on. Deterministic IDs remain available by passing a customIdGeneratortoTracerProvider(id_generator=...). This is stated in the changelog fragment.Prior art / history
random.Random()— still fork-unsafe (see the MLflow evidence above for what fixing that costs).randomremained the default.Possible follow-up (contrib repo):
AwsXRayIdGeneratorin opentelemetry-python-contrib builds its random bits from the same globalrandommodule and appears to inherit the same defect; worth a linked issue there once this merges.Type of change
How Has This Been Tested?
TestRandomIdGenerator.test_ids_unaffected_by_global_random_seedfails against the previous implementation — reproducing When the random seed is set, it causes duplicate traceId and spanId. #4376's exact duplicate IDs (164207228320579316746596838417247989971/273610340023782072underseed(10)) — and passes with this change.opentelemetry-sdkunit-test suite run before and after: identical results except the added test.tests/trace/test_trace.py,tests/trace/composite_sampler/,tests/trace/test_sampling.py) run repeatedly with no flakes.Does This PR Require a Contrib Repo Change?
Checklist:
.changelog/<PR>.fixed)