Skip to content
This repository has been archived by the owner on Jun 10, 2020. It is now read-only.

WIP ASP.NET Core Pert Fixes - 2 #907

Merged
merged 42 commits into from
Jul 11, 2019
Merged

WIP ASP.NET Core Pert Fixes - 2 #907

merged 42 commits into from
Jul 11, 2019

Conversation

Dmitry-Matveev
Copy link
Member

WIP PR to double-check logic before I start merging with develop and utilize feature flags:

  • Merged MvcDiagnosticsListener into HostingDiagnosticsListener, deprecated the former, kept it to preserve public API surface. Awesome perf gain - 20+ objects are not created, 20+ callbacks are not initiated per each request;
  • HostingDiagnosticListener now provides a constructor with Telemetry Configuration to allow it listen on Sampling Rate changes;
  • We only lookup AppId if we see new instrumentation key, not always;
  • Controversial: We only set App ID in response headers if request has incoming AppId (May need to hide before a feature flas for App Service because it breaks one-sided JS correlation in cases where server does not allow accepting incoming Request-Context header);
  • After Request object is initialized, if it has Operation Id, an attempt to figure out sampling score is made. If it's more than the currently known sampling rate, the item is proactively sampled out and we may skip certain heavy operations;
  • If-else for events prooved a bit more performant than switch-case which was compliled into GetHashCode();
  • TelemetryConfigurationOptions has an example on how to configure Proactive Sampling accordingly. It will be excluded or abstracted in the final merge, so that App Service integration can enable it at will.

The rest of PR is some auto-generated files update.

P.S. Request objects used to be leveraging Object Pool in this PR, but Hosting Diagnostics listener returns them back to pool too early (after Processors but before Channel stopped using ITelemetry item), so it requires making Pool visible from inside Channels to return appropriately or it requires cloning only sampled in items and returning original request back to object pool before channels.

  • I ran Unit Tests locally.
  • CHANGELOG.md updated with one line description of the fix, and a link to the original issue if available (Will update before merge after PR scope if fully determined)

Liudmila Molkova and others added 24 commits March 19, 2019 18:13
Remove DiagnosticAdapter and use DiagnosticListener directly
a776a5e
minor fixes
82f492c
switch
44e550e
@Dmitry-Matveev
Initialize IKey
ac85c0c
@Dmitry-Matveev
Merge branch 'lmolkova/NoDiagnosticAdapter' into dmitmatv_netcoreperf
997b4e1
@Dmitry-Matveev
Fix typo
bf33cad
@Dmitry-Matveev
GetUri with Concat
3287780
@Dmitry-Matveev
Set Header fixes, use Properties for AspNetCoreEnv.
2e3f01a
@Dmitry-Matveev
SetHeader fixes
a3fd493
@Dmitry-Matveev
merge develop
1450fa4
@Dmitry-Matveev
Test Fixes - 1
c039272
fix tests
03d7d25
@Dmitry-Matveev
Keep Public vs. Internal
293c29a
@Dmitry-Matveev
Changelog update
ff059fd
@Dmitry-Matveev
Merge branch 'develop' into dmitmatv_netcoreperf
ca3ba04
@Dmitry-Matveev
Listeners, SetAppId, Ifs vs. Switch, RequestPool
1b37346
@Dmitry-Matveev
Join listeners.
b0f7ba1
@Dmitry-Matveev
Refactor pre-sampling
48061de
@Dmitry-Matveev
Remove request pool
7cbaf82
@Dmitry-Matveev
Update AI SDK
16b33d8
@Dmitry-Matveev
Remove unnecessary code and files
2ead565
@Dmitry-Matveev
Repalce MVC Listener in tests
56808ac
@Dmitry-Matveev
Fix unit tests
1121b0f
@Dmitry-Matveev
Fix Nuget Config
6dc929e
lmolkova
lmolkova reviewed Jun 21, 2019
View reviewed changes
...plicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs Outdated Show resolved Hide resolved
...plicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs Outdated Show resolved Hide resolved
...plicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs Outdated Show resolved Hide resolved
...plicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs Outdated Show resolved Hide resolved
...plicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs Outdated Show resolved Hide resolved
....ApplicationInsights.AspNetCore/DiagnosticListeners/Implementation/MvcDiagnosticsListener.cs Outdated Show resolved Hide resolved
src/Microsoft.ApplicationInsights.AspNetCore/Microsoft.ApplicationInsights.AspNetCore.csproj Outdated Show resolved Hide resolved
src/Microsoft.ApplicationInsights.AspNetCore/Microsoft.ApplicationInsights.AspNetCore.csproj Outdated Show resolved Hide resolved
src/Microsoft.ApplicationInsights.AspNetCore/RequestTrackingTelemetryModule.cs Outdated Show resolved Hide resolved
src/Microsoft.ApplicationInsights.AspNetCore/RequestTrackingTelemetryModule.cs Outdated Show resolved Hide resolved
cijothomas
cijothomas suggested changes Jul 2, 2019
View reviewed changes
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 comments. continuing to review this....

...plicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs Outdated
// Only reply back with AppId if we got an indication that we need to set one
if (!string.IsNullOrWhiteSpace(requestTelemetry.Source))
{
SetAppIdInResponseHeader(httpContext, requestTelemetry);
Copy link
Contributor

Choose a reason for hiding this comment

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

if we chose to replyback with our app-id only when source only does the same, - i propose to make similar change in asp.net as well to be consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

As we discussed couple weeks ago, this actually breaks the existing propagation of appId back to JavaScript if JavaScript could not submit a header "Request-Context" due to header restriction. The suggestion was to put it behind another feature flag "AppServiceONBD" where we can enable it. What do you think?

...plicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs Outdated Show resolved Hide resolved
...plicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs Outdated Show resolved Hide resolved
...plicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs Show resolved Hide resolved
...plicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs Show resolved Hide resolved
...icrosoft.ApplicationInsights.AspNetCore/Implementation/TelemetryConfigurationOptionsSetup.cs Show resolved Hide resolved
...plicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs Show resolved Hide resolved
Dmitry-Matveev
Dmitry-Matveev commented Jul 2, 2019
View reviewed changes
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/answers are posted as well, please take a look.

...plicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs Show resolved Hide resolved
...plicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs Outdated Show resolved Hide resolved
...plicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs Outdated
// Only reply back with AppId if we got an indication that we need to set one
if (!string.IsNullOrWhiteSpace(requestTelemetry.Source))
{
SetAppIdInResponseHeader(httpContext, requestTelemetry);
Copy link
Member Author

Choose a reason for hiding this comment

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

As we discussed couple weeks ago, this actually breaks the existing propagation of appId back to JavaScript if JavaScript could not submit a header "Request-Context" due to header restriction. The suggestion was to put it behind another feature flag "AppServiceONBD" where we can enable it. What do you think?

...plicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs Outdated Show resolved Hide resolved
...plicationInsights.AspNetCore/DiagnosticListeners/Implementation/HostingDiagnosticListener.cs Show resolved Hide resolved
...icrosoft.ApplicationInsights.AspNetCore/Implementation/TelemetryConfigurationOptionsSetup.cs Show resolved Hide resolved
@Dmitry-Matveev Dmitry-Matveev merged commit e2f7731 into develop Jul 11, 2019
Mertsch
Mertsch reviewed Jul 19, 2019
View reviewed changes
test/ApplicationInsightsTypes/Generated/ContextTagKeys_types.cs
@@ -115,7 +115,7 @@ public partial class ContextTagKeys
[global::Bond.Id(505)]
public string UserAccountId { get; set; }

[global::Bond.Attribute("Description", "The browser's user agent string as reported by the browser. This property will be used to extract information regarding the customer's browser but will not be stored. Use custom properties to store the original user agent.")]
[global::Bond.Attribute("Description", "The browser's user agent string as reported by the browser. This property will be used to extract informaiton regarding the customer's browser but will not be stored. Use custom properties to store the original user agent.")]
Copy link

Choose a reason for hiding this comment

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

Informaiton?

test/ApplicationInsightsTypes/Generated/ContextTagKeys_types.cs
@@ -135,7 +135,7 @@ public partial class ContextTagKeys
[global::Bond.Id(705)]
public string CloudRole { get; set; }

[global::Bond.Attribute("Description", "Name of the instance where the application is running. Computer name for on-premises, instance name for Azure.")]
[global::Bond.Attribute("Description", "Name of the instance where the application is running. Computer name for on-premisis, instance name for Azure.")]
Copy link

Choose a reason for hiding this comment

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

premises is correct?!

test/ApplicationInsightsTypes/Generated/MessageData_types.cs
@@ -28,9 +28,9 @@ namespace AI
{
using System.Collections.Generic;

[global::Bond.Attribute("Description", "Instances of Message represent printf-like trace statements that are text-searched. Log4Net, NLog and other text-based log file entries are translated into instances of this type. The message does not have measurements.")]
[global::Bond.Attribute("Description", "Instances of Message represent printf-like trace statements that are text-searched. Log4Net, NLog and other text-based log file entries are translated into intances of this type. The message does not have measurements.")]
Copy link

Choose a reason for hiding this comment

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

instances is correct?! Review of the wording of all changes here?

test/ApplicationInsightsTypes/Generated/RemoteDependencyData_types.cs
@@ -59,7 +59,7 @@ public partial class RemoteDependencyData
[global::Bond.Id(61), global::Bond.Required]
public string duration { get; set; }

[global::Bond.Attribute("Description", "Indication of successful or unsuccessful call.")]
[global::Bond.Attribute("Description", "Indication of successfull or unsuccessfull call.")]
Copy link

Choose a reason for hiding this comment

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

Just one "l" as before

test/ApplicationInsightsTypes/Generated/RequestData_types.cs
@@ -63,7 +58,7 @@ public partial class RequestData
[global::Bond.Id(60), global::Bond.Required]
public string responseCode { get; set; }

[global::Bond.Attribute("Description", "Indication of successful or unsuccessful call.")]
[global::Bond.Attribute("Description", "Indication of successfull or unsuccessfull call.")]
Copy link

Choose a reason for hiding this comment

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

same here

@lmolkova lmolkova deleted the dmitmatv_netcoreperf branch October 4, 2019 22:15
@lmolkova lmolkova restored the dmitmatv_netcoreperf branch October 4, 2019 22:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Mertsch Mertsch Mertsch left review comments

@lmolkova lmolkova lmolkova left review comments

@cijothomas cijothomas cijothomas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Dmitry-Matveev @lmolkova @cijothomas @Mertsch