feat(opentelemetry-sdk-trace-base): add AlwaysRecordSampler#6168
feat(opentelemetry-sdk-trace-base): add AlwaysRecordSampler#6168majanjua-amzn wants to merge 1 commit intoopen-telemetry:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6168 +/- ##
=======================================
Coverage 95.40% 95.41%
=======================================
Files 317 318 +1
Lines 9439 9458 +19
Branches 2187 2190 +3
=======================================
+ Hits 9005 9024 +19
Misses 434 434
🚀 New features to boost your workflow:
|
majanjua-amzn
force-pushed
the
main
branch
3 times, most recently
from
14af9c0 to
ecc74b0
Compare
|
Thanks for the contribution to the opentelemetry-js project :) If this new sampler is part of the spec. Is there any motivation to have it in a dedicated package instead of having it within |
|
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
| public static create(rootSampler: Sampler): AlwaysRecordSampler { | ||
| return new AlwaysRecordSampler(rootSampler); | ||
| } | ||
|
|
||
| private constructor(rootSampler: Sampler) { | ||
| if (rootSampler == null) { | ||
| throw new Error('rootSampler is null/undefined. It must be provided'); | ||
| } | ||
| this.rootSampler = rootSampler; | ||
| } |
There was a problem hiding this comment.
Why are we using a static factory method here? Seems like extra boilerplate
| return `AlwaysRecordSampler{${this.rootSampler.toString()}}`; | ||
| } | ||
|
|
||
| wrapResultWithRecordOnlyResult(result: SamplingResult) { |
There was a problem hiding this comment.
| wrapResultWithRecordOnlyResult(result: SamplingResult) { | |
| private _wrapResultWithRecordOnlyResult(result: SamplingResult) { |
| // Includes work from: | ||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 |
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)
pichlermarc
left a comment
pichlermarc
left a comment
There was a problem hiding this comment.
@majanjua-amzn thanks for working on this. A few suggestions on the code, overall this implementation looks good.
suggestion: let's move this into @opentelemetry/sdk-trace-base from the get-go since the spec has been stablized already.
| * limitations under the License. | ||
| */ | ||
|
|
||
| export { AlwaysRecordSampler } from './AlwaysRecordSampler'; |
There was a problem hiding this comment.
what we've started doing in new packages is not exporting the classes directly, and instead export factory functions like function createAlwaysRecordSampler(options: {rootSampler: Sampler}): Sampler {}, the reason for that is hiding the concrete type from the public export, which usually becomes incompatible with itself when new private properties are added to it.
suggestion: export a factory function instead.
| let mockedSampler: Sampler; | ||
| let sampler: AlwaysRecordSampler; | ||
|
|
||
| describe('AlwaysRecordSamplerTest', () => { |
There was a problem hiding this comment.
suggestion: let's not use arrow functions with mocha. ref https://mochajs.org/features/arrow-functions/
| describe('AlwaysRecordSamplerTest', () => { | |
| describe('AlwaysRecordSamplerTest', function () { |
4 participants
Which problem is this PR solving?
Part of open-telemetry/opentelemetry-specification#4698
Introduces a way for users to access the RECORD_ONLY functionality of the sampler, as currently all available samplers either provide a RECORD_AND_SAMPLE or DROP response.
Short description of the changes
Type of change
How Has This Been Tested?
Unit tests and extensively used in aws-otel-js-instrumentation
Checklist: