Add experimental composite sampler #4714
Conversation
There was a problem hiding this comment.
Added a bunch of comments, I have yet to review the PR after looking at the doc here
https://opentelemetry.io/docs/specs/otel/trace/tracestate-probability-sampling/
| import random | ||
|
|
||
|
|
||
| def random_trace_id() -> int: |
There was a problem hiding this comment.
Maybe you can use RandomIdGenerator.generate_trace_id from opentelemetry.sdk.trace.id_generator?
opentelemetry-sdk/src/opentelemetry/sdk/trace/_sampling_experimental/__init__.py
Show resolved
Hide resolved
| @@ -0,0 +1,37 @@ | |||
| from typing import Optional, Sequence | |||
There was a problem hiding this comment.
On new files we tend to prefer importing from __future__ import annotations and use | None instead of Optional
| _threshold: int | ||
| _description: str | ||
|
|
||
| def __init__(self, sampling_probability: float): |
There was a problem hiding this comment.
Thank you for moving this PR!
Please could you add docstrings to these new functions, which would be extra helpful when the consistent probabilistic sampler spec is new.
For this one, you could copy relevant parts from Requirements for the basic samplers and potentially link/mention OTEPS
There was a problem hiding this comment.
I have added docs to the public functions including those details from the sampler. Let me know if there's anything more we can add
| sampled = False | ||
| adjusted_count_correct = False | ||
|
|
||
| decision = Decision.RECORD_AND_SAMPLE if sampled else Decision.DROP |
There was a problem hiding this comment.
I don't see it in my reading of the spec so far, but were there any discussions elsewhere about the outcome Decision.RECORD_ONLY?
There was a problem hiding this comment.
Yes. According to the current version of the specification, RECORD_ONLY decision is never generated by Consistent Probability Samplers. Any cases which require such decisions will require customizations.
| parent_span = get_current_span(parent_ctx) | ||
| parent_span_ctx = parent_span.get_span_context() | ||
| is_root = not parent_span_ctx.is_valid | ||
| if is_root: |
There was a problem hiding this comment.
There is a chance of attribute error, so this could be more like og ParentBased:
| parent_span = get_current_span(parent_ctx) | |
| parent_span_ctx = parent_span.get_span_context() | |
| is_root = not parent_span_ctx.is_valid | |
| if is_root: | |
| parent_span_ctx = get_current_span( | |
| parent_ctx | |
| ).get_span_context() | |
| if parent_span_ctx is not None and parent_span_ctx.is_valid: |
There was a problem hiding this comment.
Shouldn't the span context be either a valid one, or the invalid one but never None? I see the parent based one seems to have that check, but it doesn't match the type annotations, so unless there's a known reason to doubt the typing, I like to follow them if possible
There was a problem hiding this comment.
Thanks all. Actually I had realized while I was looking at the tracestate handling section of the spec, I completely missed the SDK portions... So I have compared with that and renamed things to match it. Notably, the word consistent isn't used much and the concept seems to composite sampling.
One note is I made an editorial decision on the public API - one thing I noticed with the sampling.py one is some samplers are constants, others are classes, which seemed inconsistent. Here I hid all the classes and only expose functions to be able to use singleton or not as needed with a consistent surface. Happy to go with anything the maintainers prefer though.
anuraaga
changed the title
This is a reopening of open-telemetry/opentelemetry-python-contrib#3668 as was recommended to target this repo.
Looking at some other similar
_exports for experimental features, they seem to be complete concepts (i.e. logs), while this is a type of an existing concept, sampler. So I tried the_sampling_experimentalname to clarify that it is an experimental part of sampling. Let me know any thoughts.Description
Adds an implementation of consistent samplers
https://opentelemetry.io/docs/specs/otel/trace/tracestate-probability-sampling/
https://opentelemetry.io/docs/specs/otel/trace/sdk/#built-in-composablesamplers
Based on the Java implementation
https://github.com/open-telemetry/opentelemetry-java-contrib/tree/main/consistent-sampling/src/main/java/io/opentelemetry/contrib/sampler/consistent56
Some differences from Java
TraceState)/cc @xrmx @tammy-baylis-swi
/cc @PeterF778 as original author in Java if interested
Type of change
How Has This Been Tested?
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.