Skip to content

fix(otel): Use id generator fallback#467

Open
wangyb-A wants to merge 2 commits into
mainfrom
otel-id-gen
Open

fix(otel): Use id generator fallback#467
wangyb-A wants to merge 2 commits into
mainfrom
otel-id-gen

Conversation

@wangyb-A

@wangyb-A wangyb-A commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Import TracerProvider from opentelemetry.sdk.trace instead of the API
base class so id_generator and sampler attributes resolve correctly
under mypy. Make DeterministicIdGenerator subclass RandomIdGenerator
for type compatibility with the provider's id_generator attribute.

Change DeterministicIdGenerator to fall back to the tracer provider's
original id generator rather than always creating a new
RandomIdGenerator. The plugin now captures the provider's existing
generator and passes it as the fallback before overwriting it.

Add unit tests covering the fallback behavior and update existing
tests to reference the renamed _fallback_id_generator attribute.

Issue #, if available: close #426

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@wangyb-A wangyb-A requested a review from zhongkechen June 10, 2026 23:52
@wangyb-A wangyb-A force-pushed the otel-id-gen branch 2 times, most recently from c5166fe to a200401 Compare June 10, 2026 23:54
zhongkechen
zhongkechen previously approved these changes Jun 11, 2026
@zhongkechen zhongkechen dismissed their stale review June 11, 2026 00:05

build failed

@wangyb-A wangyb-A force-pushed the otel-id-gen branch 3 times, most recently from a0c53e9 to 577905a Compare June 11, 2026 00:12
Import TracerProvider from opentelemetry.sdk.trace instead of the API
base class so id_generator and sampler attributes resolve correctly
under mypy. Make DeterministicIdGenerator subclass RandomIdGenerator
for type compatibility with the provider's id_generator attribute.

Change DeterministicIdGenerator to fall back to the tracer provider's
original id generator rather than always creating a new
RandomIdGenerator. The plugin now captures the provider's existing
generator and passes it as the fallback before overwriting it.

Add unit tests covering the fallback behavior and update existing
tests to reference the renamed _fallback_id_generator attribute.
@wangyb-A wangyb-A marked this pull request as ready for review June 11, 2026 19:58
@wangyb-A wangyb-A self-assigned this Jun 11, 2026
@wangyb-A wangyb-A changed the title fix(otel): Correct TracerProvider type and id generator fallback fix(otel): Use id generator fallback Jun 12, 2026
@wangyb-A wangyb-A requested a review from zhongkechen June 12, 2026 20:28
@@ -67,19 +67,25 @@ def operation_id_to_span_id(operation_id: str) -> int:

class DeterministicIdGenerator(RandomIdGenerator):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The base class was to make type checker happy. But I feel it's not correct

self._random_id_generator = RandomIdGenerator()
self._fallback_id_generator = fallback_id_generator or RandomIdGenerator()

def set_next_span_id(self, span_id: int | None) -> None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will the threading issue be addressed in a separate PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: id generator in should fall back to the original one

2 participants