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.

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

Merged
merged 23 commits into from
Jul 5, 2019
Merged

Conversation

Dmitry-Matveev
Copy link
Member

  • Sampling Score Generator is moved to AI.dll, shortcut method to get sampling score off string value is provided;
  • Telemetry Configuration adds two methods to Get and Set last observed sampling rate for the specific telemetry item type;
  • SamplingTelemetryItemTypes flags are introduced to replace hashset operations that used to check each item for sampling applicability;
  • ISupportSampling interface was extended (and later replaced with ISupportAdvancedSampling to preserve public API surface and 2.0 major version):
    • Sampling item type is now provided;
    • IsProactivelySampledOut field to mark item as proactively sampled out is provided;
    • SupportsProactiveSampling field to mark items that do support proactive sampling approach (currently, only requests, but the code is ready to handle other types as well);
  • W3C Utliities no longer uses Guid.NewGuid() to generate ID, but leverages two instances of WeakConcurrentRandom instead;
  • Telemetry Client skips Initializers if item was proactively sampled out already (e.g. in AspNetCore Diagnostics Listener) or skips initializers after the OperationId Initializer if sampling score is higher than the sample rate;
  • Sampling Telemetry processor no longer uses Hash Sets;
  • Sampling Telemetry processor calculates overall item count of proactively sampled out items if it decides it should've sampled them in (e.g. if sampling rate was increased while item was going through initializers), this item count is later added to the next item that is sampled in of this type to correct miscalc;

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.

  • I ran Unit Tests locally
  • Changes in public surface reviewed (reviewing in this PR)
  • CHANGELOG.md updated with one line description of the fix, and a link to the original issue (Will update when we have a final scope)

Copy link
Contributor

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Left few comments.

GlobalStaticVersion.props Outdated Show resolved Hide resolved
src/Microsoft.ApplicationInsights/TelemetryClient.cs Outdated Show resolved Hide resolved
src/Microsoft.ApplicationInsights/TelemetryClient.cs Outdated Show resolved Hide resolved
src/ServerTelemetryChannel/SamplingTelemetryProcessor.cs Outdated Show resolved Hide resolved
src/Microsoft.ApplicationInsights/TelemetryClient.cs Outdated Show resolved Hide resolved
src/Microsoft.ApplicationInsights/TelemetryClient.cs Outdated Show resolved Hide resolved
src/Microsoft.ApplicationInsights/TelemetryClient.cs Outdated Show resolved Hide resolved
Copy link
Member Author

@Dmitry-Matveev Dmitry-Matveev left a 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.

GlobalStaticVersion.props Outdated Show resolved Hide resolved
{
try
if (shouldTryHeadSampling)
Copy link
Member Author

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.

src/Microsoft.ApplicationInsights/TelemetryClient.cs Outdated Show resolved Hide resolved
src/Microsoft.ApplicationInsights/TelemetryClient.cs Outdated Show resolved Hide resolved
src/Microsoft.ApplicationInsights/TelemetryClient.cs Outdated Show resolved Hide resolved
src/Microsoft.ApplicationInsights/TelemetryClient.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@cijothomas cijothomas left a 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.

Copy link
Contributor

@cijothomas cijothomas left a 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..

@Dmitry-Matveev Dmitry-Matveev merged commit 981feb7 into develop Jul 5, 2019
@TimothyMothra TimothyMothra deleted the dmitmatv_netcoreperf branch June 3, 2021 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants