-
Notifications
You must be signed in to change notification settings - Fork 293
Description
Problem
When configuring Sampling, we allow users to set both the Include and Exclude properties.
This is a logical fallacy because this is a binary decision. An item cannot be both included and excluded.
In our current implementation, the Excluded types will always win over the Included types.
(we actually have a test to guarantee this behavior).
Exclude:
ApplicationInsights-dotnet/src/ServerTelemetryChannel/SamplingTelemetryProcessor.cs
Lines 80 to 84 in 981feb7
| set | |
| { | |
| this.excludedTypesString = value; | |
| this.excludedTypesFlags = this.PopulateSamplingFlagsFromTheInput(value); | |
| } |
Include:
ApplicationInsights-dotnet/src/ServerTelemetryChannel/SamplingTelemetryProcessor.cs
Lines 99 to 103 in 981feb7
| set | |
| { | |
| this.includedTypesString = value; | |
| this.includedTypesFlags = this.PopulateSamplingFlagsFromTheInput(value); | |
| } |
Evaluation:
ApplicationInsights-dotnet/src/ServerTelemetryChannel/SamplingTelemetryProcessor.cs
Lines 282 to 296 in 981feb7
| private bool IsSamplingApplicable(SamplingTelemetryItemTypes telemetryItemTypeFlag) | |
| { | |
| if (this.excludedTypesFlags.HasFlag(telemetryItemTypeFlag)) | |
| { | |
| return false; | |
| } | |
| if (this.includedTypesFlags != SamplingTelemetryItemTypes.None && !this.includedTypesFlags.HasFlag(telemetryItemTypeFlag)) | |
| { | |
| return false; | |
| } | |
| return true; | |
| } | |
| } |
Test:
Lines 327 to 339 in 981feb7
| [TestMethod] | |
| public void IncludedDoNotOverrideExcludedFromSampling() | |
| { | |
| TelemetryTypeDoesNotSupportSampling( | |
| telemetryProcessors => | |
| { | |
| telemetryProcessors.Process(new PageViewTelemetry()); | |
| telemetryProcessors.Process(new RequestTelemetry()); | |
| return 2; | |
| }, | |
| "pageview;request", | |
| "exception;request"); | |
| } |
Proposal
I want to simplify our internal types and codify the assumptions we're making in our evaluation method.
- Store only one list of
includedTypes - If a user sets Include or Exclude, no change. (This is correct.)
- If a user sets both Include and Exclude in their config (This is incorrect.)
- Setting Exclude will always take priority over Include (no change to behavior).
- New: emit a new event source warning: "Configuration Error: Exclude will take precedence over Include.".