Conversation
84e3e1a to
6bbf745
Compare
|
We need to wait for the specs to be approved and released before merging this |
|
@xrmx spec is merged, ready for review! |
6bbf745 to
93a29f2
Compare
|
Hey folks, any update here? @xrmx perhaps? Also, seems the PR build is failing with something unrelated to my changes, perhaps needs to be rerun |
42Questions
left a comment
There was a problem hiding this comment.
Had some questions regarding the use of passed attributes vs. result.attributes, and some of the testing.
| _root_sampler: Sampler | ||
|
|
||
| def __init__(self, root_sampler: Sampler): | ||
| if not root_sampler: |
There was a problem hiding this comment.
| if not root_sampler: | |
| if root_sampler is None: |
| This sampler will return the sampling result of the provided `_root_sampler`, unless the | ||
| sampling result contains the sampling decision `Decision.DROP`, in which case, a | ||
| new sampling result will be returned that is functionally equivalent to the original, except that | ||
| it contains the sampling decision `SamplingDecision.RECORD_ONLY`. This ensures that all |
There was a problem hiding this comment.
| it contains the sampling decision `SamplingDecision.RECORD_ONLY`. This ensures that all | |
| it contains the sampling decision `Decision.RECORD_ONLY`. This ensures that all |
| def _wrap_result_with_record_only_result( | ||
| result: SamplingResult, attributes: Attributes | ||
| ) -> SamplingResult: | ||
| return SamplingResult( | ||
| Decision.RECORD_ONLY, | ||
| attributes, | ||
| result.trace_state, | ||
| ) |
There was a problem hiding this comment.
It might make sense to in-line this since it's only called once, also is it intentional that the result.attributes are not being used?
| def _wrap_result_with_record_only_result( | |
| result: SamplingResult, attributes: Attributes | |
| ) -> SamplingResult: | |
| return SamplingResult( | |
| Decision.RECORD_ONLY, | |
| attributes, | |
| result.trace_state, | |
| ) | |
| def _wrap_result_with_record_only_result( | |
| result: SamplingResult | |
| ) -> SamplingResult: | |
| return SamplingResult( | |
| Decision.RECORD_ONLY, | |
| result.attributes, | |
| result.trace_state, | |
| ) |
| self.assertNotEqual(actual_result, root_result) | ||
| self.assertEqual(actual_result.decision, expected_decision) | ||
|
|
||
| self.assertEqual(actual_result.attributes, root_result.attributes) |
There was a problem hiding this comment.
What happens if the root sampler adds or changes attributes? This also ties into how _wrap_result_with_record_only_result doesn't use result.attributes, is this intentional?
| sampling_trace_state: TraceState = TraceState() | ||
| sampling_trace_state.add("key", sampling_decision.name) |
There was a problem hiding this comment.
add returns a new TraceState,
def add(self, key: str, value: str) -> "TraceState":
...
Returns:
A new TraceState with the modifications applied.
...
"""
so could this be:
| sampling_trace_state: TraceState = TraceState() | |
| sampling_trace_state.add("key", sampling_decision.name) | |
| sampling_trace_state = TraceState().add("key", sampling_decision.name) |
| ([#4862](https://github.com/open-telemetry/opentelemetry-python/pull/4862)) | ||
| - `opentelemetry-exporter-otlp-proto-http`: fix retry logic and error handling for connection failures in trace, metric, and log exporters | ||
| ([#4709](https://github.com/open-telemetry/opentelemetry-python/pull/4709)) | ||
| - feat: Add AlwaysRecordSampler |
There was a problem hiding this comment.
Might want to align the changelog style
| - feat: Add AlwaysRecordSampler | |
| - Add AlwaysRecordSampler |
| def get_description(self): | ||
| return ( | ||
| "AlwaysRecordSampler{" + self._root_sampler.get_description() + "}" | ||
| ) |
There was a problem hiding this comment.
| def get_description(self): | |
| return ( | |
| "AlwaysRecordSampler{" + self._root_sampler.get_description() + "}" | |
| ) | |
| def get_description(self) -> str: | |
| return f"AlwaysRecordSampler{{{self._root_sampler.get_description()}}}" |
|
This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment. |
|
Hi @majanjua-amzn , any thoughts on the last round of suggestions? |
The `AlwaysRecord` sampler has been in our development since the December 2025 release, and it has prototypes in the following languages: * Java: open-telemetry/opentelemetry-java#7877 * Go: open-telemetry/opentelemetry-go#7714 * PHP: open-telemetry/opentelemetry-php#1834 * Javascript: open-telemetry/opentelemetry-js#6168 * Python: open-telemetry/opentelemetry-python#4823 * DotNet: open-telemetry/opentelemetry-dotnet#6732 (got closed but hopefully we bring it back)
Description
Adds a new AlwaysRecordSampler as per:
This sampler behaves the same as it's root sampler, with the exception that it will replace any DROP decisions with an equivalent RECORD one, while maintaining the attributes and trace state from the decision created by the root sampler.
Also renamed the folder holding the tests for experimental sampling features from
composite_samplertosampling_experimental, as more than just the composite sampler is being tested in this folder, respecting the organization of the files in the src folder.Type of change
How Has This Been Tested?
Unit tests
Does This PR Require a Contrib Repo Change?
Checklist: