Fix Samplers to match spec #1037
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1037 +/- ##
==========================================
- Coverage 77.12% 77.08% -0.05%
==========================================
Files 221 221
Lines 6156 6092 -64
==========================================
- Hits 4748 4696 -52
+ Misses 1408 1396 -12
|
src/OpenTelemetry/Trace/Decision.cs
Outdated
| /// <summary> | ||
| /// Enumeration to define sampling decision. | ||
| /// </summary> | ||
| public enum Decision |
There was a problem hiding this comment.
I'm concerned about having this name under OpenTelemetry.Trace:
Decisioncould be used by other namespaces given it is a common word.- It is not clear literally what does
OpenTelemetry.Trace.Decisionmean.
This is what OpenTelemetry Python is doing and we might borrow some idea.
There was a problem hiding this comment.
My suggestion would be:
- Introduce
OpenTelemetry.Trace.Samplingnamespace. - Put the
Samplerbase class,Decisionand all the samplers under that namespace. - Folks who don't use sampler explicitly don't need to have
using OpenTelemetry.Trace.Sampling. - Folks who use sampler or write sampler will need to have
using OpenTelemetry.Trace.Sampling.
An alternative approach would be calling this class SamplingDecision, but that seems to be worse?
There was a problem hiding this comment.
Agree with @reyang it's a bit too generic to have in a common namespace. I think SamplingDecision is a good change to make regardless of where it goes. Decision alone is hyper-generic 🤣
There was a problem hiding this comment.
I would prefer SamplingDecision as the class name if we agree to move the current samplers from src/Trace/Samplers to src/Trace and remove the OpenTelemetry.Trace.Samplers namespace 😃
(the movement of existing samplers doesn't have to be in this PR)
There was a problem hiding this comment.
Moved the current samplers from src/Trace/Samplers to src/Trace and removed the OpenTelemetry.Trace.Samplers namespace. Using SamplingDecision as the enum name.
test/OpenTelemetry.Tests/Implementation/Trace/Export/BatchingActivityProcessorTests.cs
Outdated
Show resolved
Hide resolved
test/OpenTelemetry.Tests/Implementation/Trace/Export/SimpleActivityProcessorTest.cs
Outdated
Show resolved
Hide resolved
Fixes #941 .
Changes
Please provide a brief description of the changes here. Update the
CHANGELOG.mdfor non-trivial changes.SamplingDecisionenum in Sampling result to match the spec. This has a 1:1 mapping withActivityDataRequest. CreatedSamplingDecisionenum instead of usingActivityDataRequest, to keep enum names simple and to match OT spec. This way it will be easier to write custom sampler.SamplingDecision.RecordAndSampled->ActivityDataRequest.AllDataAndRecordedSamplingDecision.Record->ActivityDataRequest.AllDataSamplingDecision.NotRecord->ActivityDataRequest.PropagationDataSimpleActivityProcessorandBatchingActivityProcessorto export data only ifActivity.Recordedis true.ActivityListenercalls activity processor only whenSamplingDecisionisSamplingDecision.RecordorSamplingDecision.RecordAndSampledFor significant contributions please make sure you have completed the following items: