Added AlwaysRecordSampler

[majanjua-amzn]

Background

Spec PR: open-telemetry/opentelemetry-specification#4699

Design discussion issue open-telemetry/opentelemetry-specification#4698

Changes

• Add AlwaysRecordSampler that replaces any Drop sampling decision provided by a delegate sampler with RecordOnly
• Added appropriate unit tests using the TestSampler as a delegate

Changes copied from the following with some modifications: aws-otel-dotnet-instrumentation

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)

[Kielek]

Request change to avoid incidental merge.

For now, it is a specification proposal, it means we are not able to merge it even to the unstable releases.
If accepted as a development, needs to follow comments from #6617 (comment)

[majanjua-amzn]

@Kielek spec PR is merged, ready for review!

[cijothomas]

Request change to avoid incidental merge.

For now, it is a specification proposal, it means we are not able to merge it even to the unstable releases. If accepted as a development, needs to follow comments from #6617 (comment)

Would it be easier to host this in https://github.com/open-telemetry/opentelemetry-dotnet-contrib/tree/main/src/OpenTelemetry.Extensions given this is a somewhat standalone component (a sampler) ? Once spec is stable, it can be moved to main repo.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.86%. Comparing base (d855a7a) to head (f912bdf).
⚠️ Report is 4 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6732      +/-   ##
==========================================
+ Coverage   86.85%   86.86%   +0.01%     
==========================================
  Files         258      259       +1     
  Lines       11990    12001      +11     
==========================================
+ Hits        10414    10425      +11     
  Misses       1576     1576              

Flag	Coverage Δ
unittests-Project-Stable	86.49% <100.00%> (-0.23%)	⬇️
unittests-UnstableCoreLibraries-Experimental	86.07% <ø> (ø)

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

Files with missing lines	Coverage Δ
...OpenTelemetry/Trace/Sampler/AlwaysRecordSampler.cs	100.00% <100.00%> (ø)

[Kielek]

Couple of suggestions.

The idea adding to OpenTelemetry.Extension is worth to consider, but I would not block adding it also here. Without stabilizing specification it will be barely usable by the normal OpenTelemetry SDK users.

[Kielek]

Suggested change

	* Added `AlwaysRecordSampler`.
	* **Experimental (pre-release builds only):** Added `AlwaysRecordSampler`.

[Kielek]

Needs some real data here.

[Kielek]

The same problem we have in the ParentBasedSample

Suggested change

	Guard.ThrowIfNull(rootSampler);
	return new AlwaysRecordSampler(rootSampler);
	Guard.ThrowIfNull(rootSampler);
	#pragma warning disable CA1062 // Validate arguments of public methods - needed for netstandard2.1
	return new AlwaysRecordSampler(rootSampler);
	#pragma warning restore CA1062 // Validate arguments of public methods - needed for netstandard2.1

[cijothomas]

Couple of suggestions.

The idea adding to OpenTelemetry.Extension is worth to consider, but I would not block adding it also here. Without stabilizing specification it will be barely usable by the normal OpenTelemetry SDK users.

That made me realize one thing - there is no document on how to enable experimental features.

Keeping in Extension package has one advantage - users can continue to be in stable releases with the exception of that single nuget package. Alternative is to move to unstable release of every package. For features like this, which is truly standalone component, I think the extension package is an easier route. For other things like LogRecord/Exemplars, which require core API change in the Sdk itself, we should continue to use the current approach.

[rajkumar-rangaraj]

I agree with @cijothomas that it's too soon to include this in the SDK given the current status of the spec. I recommend adding it to OpenTelemetry.Extensions.