Support for SamplerDetectors and EnvVarSamplerDetector #3635
Conversation
Kielek
force-pushed
the
sampler-from-env-variable
branch
from
be986a8 to
973f289
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3635 +/- ##
==========================================
- Coverage 87.35% 86.94% -0.42%
==========================================
Files 282 285 +3
Lines 10132 10179 +47
==========================================
- Hits 8851 8850 -1
- Misses 1281 1329 +48
|
| @@ -30,7 +30,7 @@ public static void Main() | |||
| { | |||
| using var tracerProvider = Sdk.CreateTracerProviderBuilder() | |||
| .AddSource("MyCompany.MyProduct.MyLibrary") | |||
| .SetSampler(new AlwaysOnSampler()) | |||
| .ConfigureSampler(x => x.SetDefaultSampler(new AlwaysOnSampler())) | |||
There was a problem hiding this comment.
I think it should be
.ConfigureSampler(x => x.Clear().SetDefaultSampler(new AlwaysOnSampler()))Can you elaborate on why you suggest using the "builder API" instead of SetSampler if the user would like to always use one explicit sampler?
There was a problem hiding this comment.
Fixed in fb11564.
New API provide broader functionalities so it should be used in all examples in the repository.
|
|
||
| double ratio = defaultRatio; | ||
|
|
||
| if (hasValue && double.TryParse(envVarValue, out double parsedRatio) && parsedRatio >= 0.0 && parsedRatio <= 1.0) |
There was a problem hiding this comment.
We should throw a FormatException if the ratio is bad. Example in other place:
There was a problem hiding this comment.
I have tried to avoid throwing exceptions.
I think that both solutions should be fine.
@open-telemetry/dotnet-approvers, any preferences/comments what is the better option?
There was a problem hiding this comment.
Take notice that EnvironmentVariableHelper is throwing FormatException when it fails to parse the value.
test/OpenTelemetry.Tests/Trace/Sampling/OTelEnvSamplerDetectorTests.cs
Outdated
Show resolved
Hide resolved
| @@ -335,6 +335,22 @@ Custom resource detectors can be implemented: | |||
|
|
|||
| A demo ResourceDetector is shown [here](./MyResourceDetector.cs). | |||
|
|
|||
| ## Sampler Detector | |||
|
|
|||
| OpenTelemetry .NET SDK provides a sampler detector for detecting sampler | |||
There was a problem hiding this comment.
mm.. I am not sure of the need of such a new api. Can we simply modify SDK to automatically pick Sampler from Env Variables?
There was a problem hiding this comment.
I see some benefits for having builder with detectors:
- possibility to programmatically disable env. vars.
- easily add more samplers supported by the env. vars. You can just implement externally support for other samplers without taking care about parsing values for common samplers. Please consider something like
XRayOTelEnvVarSamplerDetector. With current design it could be chained with current CommonEnvVar Detector.
There was a problem hiding this comment.
There are issues with the existing environment variable stuff. Primarily, it doesn't work with IConfiguration/IOptions. This is something I am looking at, hoping to find a new pattern that smooths all of this out. Probably first to drive #3583. I would vote to wait on this sorry @Kielek I know that is probably frustrating!
There was a problem hiding this comment.
If we speaking about timeframe, it will be great to include it at least based env. var. support for 1.4.0 release. We would like to give AutoInstrumentation users some flexibility here.
I know that we will need more thing configurable in the future so I will observe issue you have mentioned.
|
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. |
github-actions
bot
added
the
Stale
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Fixes #3601.
Changes
Please provide a brief description of the changes here.
SamplerDetectors functionality (based on what was implemented forResources)OTEL_TRACES_SAMPLERandOTEL_TRACES_SAMPLER_ARGenv vars foralways_on,always_off,traceidratio,parentbased_always_on,parentbased_always_off,parentbased_traceidratio.For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes