Second wave of performance fixes#1158
Conversation
PublicAPI/Microsoft.ApplicationInsights.dll/net45/PublicAPI.Unshipped.txt
Show resolved
Hide resolved
PublicAPI/Microsoft.ApplicationInsights.dll/net45/PublicAPI.Unshipped.txt
Outdated
Show resolved
Hide resolved
Test/Microsoft.ApplicationInsights.Test/Shared/TelemetryClientTest.cs
Outdated
Show resolved
Hide resolved
Test/Microsoft.ApplicationInsights.Test/Shared/TelemetryClientTest.cs
Outdated
Show resolved
Hide resolved
Dmitry-Matveev
left a comment
There was a problem hiding this comment.
@cijothomas , addressed most of the comments, some questions on couple, please take a look.
Test/Microsoft.ApplicationInsights.Test/Shared/TelemetryClientTest.cs
Outdated
Show resolved
Hide resolved
Test/Microsoft.ApplicationInsights.Test/Shared/TelemetryClientTest.cs
Outdated
Show resolved
Hide resolved
Test/ServerTelemetryChannel.Test/Shared.Tests/SamplingTelemetryProcessorTest.cs
Show resolved
Hide resolved
| if (!sampledOut) | ||
| { | ||
| try | ||
| if (shouldTryHeadSampling) |
There was a problem hiding this comment.
That's the only open question - Sergey suggested all public to be "Head..." and we are having troubles properly naming IsProactiveSampledOut in this way. As soon as new name pops in our heads, I'll replace everything with Heads.
cijothomas
left a comment
There was a problem hiding this comment.
Responded to comments. Looking good.
Haven't set Approve yet.
Would like to go over this in person so everyone in team is fully aware of the changes. Lets do it sometime tomorrow.
Remove SupportsProactiveSampling property.
cijothomas
left a comment
There was a problem hiding this comment.
please address the comment about adding comment about sampling processor compensating for dropped items in future item..
In places code looks ugly as its optimized for perf. I'm super-open to make it better while preserving perf gains, please do let me know your suggestions, we'll check perf and take those. (E.g. Interlocked caused me to have Array vs. Dictionary in one place and switch-case in the other).
The overall plan is to make proactive sampling a feature flag when I merge with develop that supports feature flags. App Service integration will enable this feature flag just like in case with the deferred QuickPulse GetURI() calculation.
One caveat for Public API surface: If someone had implemented a Telemetry Initializer / Processor that samples the item in, so this change (proactive sampling part) technically cannot be part of SDK itself, only of codeless way of attaching SDKs. That's something we'd need to discuss.
P.S. Sorry for the PR size, mostly API/Tests/DataContracts/Interface changes that spawned across multiple item types.