Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenTelemetry.Extensions.AzureMonitor - Add sampleRate attribute to SamplingResult. #623

Merged
merged 3 commits into from
Sep 7, 2022
Merged

OpenTelemetry.Extensions.AzureMonitor - Add sampleRate attribute to SamplingResult. #623

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
79da507
Add sampleRate attribute to SamplingResult.
rajkumar-rangaraj Sep 6, 2022
dcc3c46
DropSamplingResult to static
rajkumar-rangaraj Sep 7, 2022
1ac5a2b
Merge remote-tracking branch 'origin/main' into rajrang/addsampleRate
rajkumar-rangaraj Sep 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion src/OpenTelemetry.Extensions.AzureMonitor/ApplicationInsightsSampler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
// limitations under the License.
// </copyright>
using System;
using System.Collections.Generic;
using OpenTelemetry.Internal;
using OpenTelemetry.Trace;

Expand All @@ -25,6 +26,8 @@ namespace OpenTelemetry.Extensions.AzureMonitor;
/// </summary>
public class ApplicationInsightsSampler : Sampler
{
private static readonly SamplingResult DropSamplingResult = new(false);
private readonly SamplingResult recordAndSampleSamplingResult;
private readonly float samplingRatio;

/// <summary>
Expand All @@ -38,6 +41,13 @@ public ApplicationInsightsSampler(float samplingRatio)

this.samplingRatio = samplingRatio;
this.Description = "ApplicationInsightsSampler{" + samplingRatio + "}";
var sampleRate = (int)Math.Round(samplingRatio * 100);
utpilla marked this conversation as resolved.
Show resolved Hide resolved
this.recordAndSampleSamplingResult = new SamplingResult(
SamplingDecision.RecordAndSample,
new Dictionary<string, object>
{
{ "sampleRate", sampleRate },
});
}

/// <summary>
Expand All @@ -48,8 +58,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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we okay with the naive equality comparison for float?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see an issue, do you think this could cause an issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
{

}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.0000001 is an invalid valid. Guard in the constructor - Guard.ThrowIfOutOfRange((double)samplingRatio, min: 0.0, max: 1.0); should throw an exception.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.0000001 is 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
return DropSamplingResult;
}

if (this.samplingRatio == 1)
{
return this.recordAndSampleSamplingResult;
}

double sampleScore = DJB2SampleScore(samplingParameters.TraceId.ToHexString().ToLowerInvariant());
return new SamplingResult(sampleScore < this.samplingRatio);

if (sampleScore < this.samplingRatio)
{
return this.recordAndSampleSamplingResult;
}
else
{
return DropSamplingResult;
}
}

private static double DJB2SampleScore(string traceIdHex)
Expand Down