OpenTelemetry.Extensions.AzureMonitor - Add sampleRate attribute to SamplingResult. #623
Conversation
utpilla
added
the
comp:extensions.azuremonitor
| @@ -48,8 +59,26 @@ public ApplicationInsightsSampler(float samplingRatio) | |||
| /// <returns>Returns whether or not we should sample telemetry in the form of a <see cref="SamplingResult"/> class.</returns> | |||
| public override SamplingResult ShouldSample(in SamplingParameters samplingParameters) | |||
| { | |||
| if (this.samplingRatio == 0) | |||
There was a problem hiding this comment.
Are we okay with the naive equality comparison for float?
There was a problem hiding this comment.
I don't see an issue, do you think this could cause an issue?
There was a problem hiding this comment.
It depends on the sampler requirements. Do we want to sample and record for this value of sampling ratio 1.0000001?
float samplingRatio = 1.0000001F;
if (samplingRatio == 1) // <- This is not true
{
}There was a problem hiding this comment.
1.0000001 is an invalid valid. Guard in the constructor - Guard.ThrowIfOutOfRange((double)samplingRatio, min: 0.0, max: 1.0); should throw an exception.
There was a problem hiding this comment.
In general, equality comparison of floating-point values is problematic: https://docs.microsoft.com/en-us/dotnet/api/system.double?view=net-6.0#testing-for-equality
There was a problem hiding this comment.
1.0000001is an invalid valid. Guard in the constructor -Guard.ThrowIfOutOfRange((double)samplingRatio, min: 0.0, max: 1.0);should throw an exception.
What about 0.9999999F?
There was a problem hiding this comment.
This should not be treated as 1. Tested the behavior in a small test app, it works perfectly. I could understand the problem with float number, but it should not have impact on 0 and 1. As a follow up task we could add tests for this project to validate it.
There was a problem hiding this comment.
This should not be treated as 1.
I was essentially asking this.
I know that the current code does not treat it as 1. I just wanted to know if we should be treating 0.9999999F as 1.0 and 0.00000001F as 0.0 as when the user provides the samplingRatio, it might have been computed as result of other floating-point operation which might cause precision loss.
src/OpenTelemetry.Extensions.AzureMonitor/ApplicationInsightsSampler.cs
Outdated
Show resolved
Hide resolved
As specified in description TODO, want add changelog in a follow up PR as this project does not have any entry earlier. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #623 +/- ##
=======================================
Coverage 77.76% 77.76%
=======================================
Files 170 170
Lines 5163 5163
=======================================
Hits 4015 4015
Misses 1148 1148 |
Changes
Please provide a brief description of the changes here.
On successful sampling, add
sampleRateattribute toSamplingResult.TODO
CHANGELOG.mdin a follow up PR.For significant contributions please make sure you have completed the following items:
[ ] AppropriateCHANGELOG.mdupdated for non-trivial changes[ ] Design discussion issue #