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

Give Sampler the span ID #3523

Closed
adriangb opened this issue Nov 10, 2023 · 1 comment
Closed

Give Sampler the span ID #3523

adriangb opened this issue Nov 10, 2023 · 1 comment

Comments

@adriangb
Copy link
Contributor

adriangb commented Nov 10, 2023

Coming from open-telemetry/opentelemetry-specification#3205 (comment)

I'd like to give Sampler the span ID. It currently only gets the trace ID.
It should be an easy change given that the span ID is generated just a few lines down:

sampling_result = self.sampler.should_sample(
context, trace_id, name, kind, attributes, links
)
trace_flags = (
trace_api.TraceFlags(trace_api.TraceFlags.SAMPLED)
if sampling_result.decision.is_sampled()
else trace_api.TraceFlags(trace_api.TraceFlags.DEFAULT)
)
span_context = trace_api.SpanContext(
trace_id,
self.id_generator.generate_span_id(),

As far as I know, there are no guarantees about the IdGenerator being called before or after sampling.

The spec only says which arguments are required for shouldSample to accept. It's not clear to me if that means that implementations can choose to make more arguments available to implementers or if that means that all other arguments must be optional. Either way we can make the argument optional but always pass it in (although I think it'd be better if it wasn't optional in the Python sense if it's always provided).

I see various ways to implement this depending on the desire to be backward compatible with all uses.
The simplest version that just adds the argument to the classes in the SDK and call site in the API should only be a dozen lines of code or so but in theory, breaks any custom Samplers. More complex versions could mean making a SamplerV2 class or a should_sample_v2 method, or adding it to the classes in the SDK but doing a try/except around a TypeError at the API call site to support older implementations.

As far as I can tell the last breaking change made was in #1764 but the SDK vas pre v1 at that point.

@ocelotl
Copy link
Contributor

ocelotl commented Jan 5, 2024

I'd rather not implement this before the spec has made a decision on this feature. Please discuss with the spec, if this feature gets approved there, we can reopen this issue. #3574 can remain open (as a draft to avoid accidental merging) in the meantime so that you have code that can use to explain your idea.

@ocelotl ocelotl closed this as completed Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants