-
Notifications
You must be signed in to change notification settings - Fork 287
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.
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
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left few comments.
Test/Microsoft.ApplicationInsights.Test/Shared/TelemetryClientTest.cs
Outdated
Show resolved
Hide resolved
Test/Microsoft.ApplicationInsights.Test/Shared/TelemetryClientTest.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
{ | ||
try | ||
if (shouldTryHeadSampling) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.