Refactor sampler config to use `SamplerOptions` and `IOptions` by stevejgordon · Pull Request #7192 · open-telemetry/opentelemetry-dotnet

stevejgordon · 2026-04-28T06:46:55Z

Changes

Introduce SamplerOptions to encapsulate trace sampler configuration, parsing OTEL_TRACES_SAMPLER and OTEL_TRACES_SAMPLER_ARG from IConfiguration.
Refactor TracerProviderSdk to resolve sampler settings via IOptions<SamplerOptions>, enabling standard options pipeline overrides and improved diagnostics.
Add tests for configuration, overrides, and error handling.
Update public API and diagnostics to reflect new options-based approach.

Merge requirement checklist

CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
Unit tests added/updated
Appropriate CHANGELOG.md files updated for non-trivial changes
Changes in public API reviewed (if applicable)

stevejgordon · 2026-04-28T07:17:21Z

@martincostello I'm a bit baffled by these build warnings. I don't see them locally (running dotnet build ./build/OpenTelemetry.proj --configuration Release -p:ExposeExperimentalFeatures=true) and the tests pass too.

martincostello · 2026-04-28T08:22:51Z

Your branch is out-of-date following a refactor I did in #7175.

You don't see it locally because you're not building a merge commit with main.

stevejgordon · 2026-04-28T08:51:56Z

Ah darn! okay, thanks!

- Introduce `SamplerOptions` to encapsulate trace sampler configuration, parsing OTEL_TRACES_SAMPLER and OTEL_TRACES_SAMPLER_ARG from IConfiguration. - Refactor `TracerProviderSdk` to resolve sampler settings via `IOptions<SamplerOptions>`, enabling standard options pipeline overrides and improved diagnostics. - Add tests for configuration, overrides, and error handling. - Update public API and diagnostics to reflect new options-based approach.

stevejgordon · 2026-04-28T10:32:09Z

@martincostello Should all be resolved now and ready for review.

codecov · 2026-04-28T10:33:20Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.56%. Comparing base (6232fb2) to head (0eb374d).
⚠️ Report is 91 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7192      +/-   ##
==========================================
+ Coverage   89.54%   89.56%   +0.02%     
==========================================
  Files         272      273       +1     
  Lines       13301    13327      +26     
==========================================
+ Hits        11910    11936      +26     
  Misses       1391     1391

Flag	Coverage Δ
unittests-Project-Experimental	`89.45% <100.00%> (-0.03%)`	⬇️
unittests-Project-Stable	`89.18% <100.00%> (-0.30%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...lder/ProviderBuilderServiceCollectionExtensions.cs	`100.00% <100.00%> (ø)`
src/OpenTelemetry/Trace/SamplerOptions.cs	`100.00% <100.00%> (ø)`
src/OpenTelemetry/Trace/TracerProviderSdk.cs	`99.68% <100.00%> (+0.32%)`	⬆️

... and 4 files with indirect coverage changes

Kielek · 2026-04-28T11:02:55Z

+    /// <summary>
+    /// Gets or sets the sampler argument.
+    /// </summary>
+    public double? SamplerArg


I do not think that we should expose this.
You can potentially add some internal property/method to parse the value. Env. vars treats it as string.

This again was to support the use case services.Configure<SamplerOptions>(o => o.SamplerArg = 0.5) for configuring sampling in code, which today is not possible, unlike other options types.

Kielek · 2026-04-28T11:06:27Z

+/// OTEL_TRACES_SAMPLER and OTEL_TRACES_SAMPLER_ARG environment variables
+/// are parsed during object construction.
+/// </summary>
+public sealed class SamplerOptions


I have some dobuts that this class is what you really need for configuration SDK

lets consider: https://github.com/open-telemetry/opentelemetry-configuration/blob/4ad327807a9e7dc7d440dac6ca106e789df1233b/schema-docs.md#parentbasedsampler-

There is some implementation for ParentBasedSampler in https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/blob/00324dc0662996142548065278cc92c28323e1da/src/OpenTelemetry.AutoInstrumentation/Configurations/FileBasedConfiguration/SamplerFactory.cs#L129-L138

Technically you can include lets say TraceIdSampler in ParentBased. According to spec, it should be perfectly valid case.

One more thing, We can make probably some interim step. Make this class internal, and when you will be working on Configuration SDK we can adjust public contracts.

So, this type is scoped to the env-var path (flat keys) and bringing consistency to the options types in general. It isn't intended to represent the full declarative config sampler tree which can (probably) be implemented without this. I think declarative config will hook in earlier in TracerProviderSdk.GetSampler method, falling back to this when that can't provider the richer factory approach. I may need to go back and read the specs more though as it's definitely possible I'm missing something.

As for making it internal, that prevents the consumer use case of services.Configure<SamplerOptions>(o => o.SamplerArg = 0.5) which many of the other options types currently enable. The goal of adding this was feature parity after it was identified in the config analysis.

martincostello · 2026-04-28T11:00:42Z

+OpenTelemetry.Trace.SamplerOptions.SamplerArg.get -> double?
+OpenTelemetry.Trace.SamplerOptions.SamplerArg.set -> void


SamplerArgument?

If we keep this as a public type, I'm okay with the rename. It was mirroring the env var names but a clearer name is likely better and it's not hard to see how they match up.

martincostello · 2026-04-28T11:07:13Z

-        if (configuration.TryGetStringValue(TracesSamplerArgConfigKey, out var configValue) &&
-                double.TryParse(configValue, NumberStyles.Float | NumberStyles.AllowThousands, CultureInfo.InvariantCulture, out var traceIdRatio) &&
-                !double.IsNaN(traceIdRatio) &&
-                !double.IsInfinity(traceIdRatio) &&
-                traceIdRatio >= 0.0 &&
-                traceIdRatio <= 1.0)
+        var samplerArg = options.SamplerArg;
+        if (samplerArg.HasValue
+            && !double.IsNaN(samplerArg.Value)
+            && !double.IsInfinity(samplerArg.Value)
+            && samplerArg.Value >= 0.0
+            && samplerArg.Value <= 1.0)
        {
-            return traceIdRatio;
-        }
-        else
-        {
-            OpenTelemetrySdkEventSource.Log.TracesSamplerArgConfigInvalid(configValue ?? string.Empty);
+            return samplerArg.Value;
        }

+        // No valid ratio. Log the original config string for diagnostic fidelity and fall back to 1.0.
+        OpenTelemetrySdkEventSource.Log.TracesSamplerArgConfigInvalid(
+            options.SamplerArgRaw
+            ?? (samplerArg.HasValue
+                ? samplerArg.Value.ToString(CultureInfo.InvariantCulture)
+                : string.Empty));
+


I would refactor this to avoid calling .Value multiple times as there's a (small) overhead from it internally re-checking if there's a value internally so it could throw if there wasn't.

martincostello · 2026-04-28T11:08:53Z

+        using var clearSamplerEnv = EnvironmentVariableScope.Create(SamplerOptions.TracesSamplerConfigKey, null);
+        using var clearSamplerArgEnv = EnvironmentVariableScope.Create(SamplerOptions.TracesSamplerArgConfigKey, null);


You should be able to just have one by using an array of tuples:

Suggested change

using var clearSamplerEnv = EnvironmentVariableScope.Create(SamplerOptions.TracesSamplerConfigKey, null);

using var clearSamplerArgEnv = EnvironmentVariableScope.Create(SamplerOptions.TracesSamplerArgConfigKey, null);

using var scope = EnvironmentVariableScope.Create(

[

(SamplerOptions.TracesSamplerConfigKey, null),

(SamplerOptions.TracesSamplerArgConfigKey, null),

]);

martincostello · 2026-04-28T11:09:05Z

+        using var clearSamplerEnv = EnvironmentVariableScope.Create(SamplerOptions.TracesSamplerConfigKey, null);
+        using var clearSamplerArgEnv = EnvironmentVariableScope.Create(SamplerOptions.TracesSamplerArgConfigKey, null);


martincostello · 2026-04-28T11:12:00Z

+    public SamplerOptionsTests()
+    {
+        ClearSamplerEnvVars();
+    }


Let's refactor this to use EnvironmentVariableScope.

I did a similar clean-up in another PR yesterday (but I've forgotten which one to point to the code).

Found it: https://github.com/open-telemetry/opentelemetry-dotnet/pull/7187/files#diff-ab8c84357ded477e170ee51a43d4f3ecd8ba1da3295489cea089d4388e4c55b1

Co-authored-by: Martin Costello <martin@martincostello.com>

github-actions · 2026-05-06T04:24:55Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

stevejgordon · 2026-05-12T07:43:19Z

I haven't made all PR review changes yet. @Kielek what do you think on the responses to your comments on internal vs. public? If we plan to introduce the new declarative config package as part of the SDK (new project) then we could start with internal for that use case assuming we can make the internals visible to that project, however, we'd still lose the Configure<T> use case for consumers. Happy to discuss further as I'd like to unblock this one.

github-actions · 2026-05-20T04:38:22Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

stevejgordon requested a review from a team as a code owner April 28, 2026 06:46

github-actions Bot added pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package pkg:OpenTelemetry.Extensions.Hosting Issues related to OpenTelemetry.Extensions.Hosting NuGet package labels Apr 28, 2026

stevejgordon force-pushed the 7178-sampler-options branch from 54d1705 to 21b5f82 Compare April 28, 2026 10:01

stevejgordon force-pushed the 7178-sampler-options branch from 21b5f82 to 9c1c993 Compare April 28, 2026 10:24

Kielek reviewed Apr 28, 2026

View reviewed changes

martincostello reviewed Apr 28, 2026

View reviewed changes

stevejgordon and others added 3 commits April 28, 2026 12:48

Update src/OpenTelemetry/CHANGELOG.md

23bda00

Co-authored-by: Martin Costello <martin@martincostello.com>

Update src/OpenTelemetry/Trace/SamplerOptions.cs

6b335df

Co-authored-by: Martin Costello <martin@martincostello.com>

Update src/OpenTelemetry/Trace/TracerProviderSdk.cs

0eb374d

Co-authored-by: Martin Costello <martin@martincostello.com>

github-actions Bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label May 6, 2026

Kielek removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label May 6, 2026

github-actions Bot added Stale Issues and pull requests which have been flagged for closing due to inactivity and removed Stale Issues and pull requests which have been flagged for closing due to inactivity labels May 20, 2026

		OpenTelemetry.Trace.SamplerOptions.SamplerArg.get -> double?
		OpenTelemetry.Trace.SamplerOptions.SamplerArg.set -> void

		using var clearSamplerEnv = EnvironmentVariableScope.Create(SamplerOptions.TracesSamplerConfigKey, null);
		using var clearSamplerArgEnv = EnvironmentVariableScope.Create(SamplerOptions.TracesSamplerArgConfigKey, null);

-        using var clearSamplerEnv = EnvironmentVariableScope.Create(SamplerOptions.TracesSamplerConfigKey, null);
-        using var clearSamplerArgEnv = EnvironmentVariableScope.Create(SamplerOptions.TracesSamplerArgConfigKey, null);
+        using var scope = EnvironmentVariableScope.Create(
+            [
+                (SamplerOptions.TracesSamplerConfigKey, null),
+                (SamplerOptions.TracesSamplerArgConfigKey, null),
+            ]);

Conversation

stevejgordon commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Merge requirement checklist

Uh oh!

stevejgordon commented Apr 28, 2026

Uh oh!

martincostello commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevejgordon commented Apr 28, 2026

Uh oh!

stevejgordon commented Apr 28, 2026

Uh oh!

codecov Bot commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 6, 2026

Uh oh!

stevejgordon commented May 12, 2026

Uh oh!

github-actions Bot commented May 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stevejgordon commented Apr 28, 2026 •

edited

Loading

martincostello commented Apr 28, 2026 •

edited

Loading

codecov Bot commented Apr 28, 2026 •

edited

Loading