Performance Enhancement: Request Tracking Properties & Post Sampling Execution Deferral

[TimothyMothra]

@Mikhail-Pranovich identified performance issues in RequestTrackingTelemetryModule.
Specifically:

1. Setting the requestTelemetry.Url from the context
   https://github.com/Microsoft/ApplicationInsights-dotnet-server/blob/3bee86d3f7ec3e81efe6c5ad3ebdbe37f79929bb/Src/Web/Web.Shared.Net/RequestTrackingTelemetryModule.cs#L164-L167

2. Setting the requestTelemetry.Source to the SourceAppId
   https://github.com/Microsoft/ApplicationInsights-dotnet-server/blob/3bee86d3f7ec3e81efe6c5ad3ebdbe37f79929bb/Src/Web/Web.Shared.Net/RequestTrackingTelemetryModule.cs#L176-L203

Proposal
I'm proposing a feature flag to make this an opt-in feature:

1. A new property: RequestTrackingTelemetryModule.DisableTrackingProperties (open to naming suggestions)
   https://github.com/Microsoft/ApplicationInsights-dotnet-server/blob/944ee584e39b79b6707479cc3762e5077f97b8e5/Src/Web/Web.Shared.Net/RequestTrackingTelemetryModule.cs#L56-L64

This would disable setting this properties in the RequestTrackingTelemetryModule.

2. Settings these properties would be deferred to a TelemetryProcessor:
   https://github.com/Microsoft/ApplicationInsights-dotnet-server/blob/f145561f564ce3094115c5588920fc7eef0f8466/Src/Web/Web.Shared.Net/Extensibility/Implementation/PostSamplingTelemetryProcessor.cs#L12-L17

Risks
@cijothomas had a good comment:

URL is already populated by RequestCollectionModules which is shipped officially.
Modifying existing RequestCollectionModules to NOT populate URL, and instead rely on this new TP is a behavior change- Tel.Initializers will not see the URL. And also any TP added before this TP wont see URL. And there is no guarantee that TP is run in same context as incoming Request, so HttpContext.Current could be null.

We can discuss such change, but i'd do it only when we are changing major version.

Open Question

• I'm looking for a compromise. What changes would be required to contribute this change to the official SDK?
  • Attach wants these changes already, and cannot wait for 3.0

[TimothyMothra]

I'm shocked that populating the Request.Unvalidated.Url adds a significant performance overhead.

I also see that removing it adds additional side effects...

• TelemetryProcessors are not guaranteed to run on the same thread as the original request.
  • TODO: Need to check that HttpContext.Current is not null
• removing it from the RequestTrackingTelemetryModule Requires additional changes to QuickPulseTelemetryProcessor
  https://github.com/Microsoft/ApplicationInsights-dotnet-server/blob/e6f7685b026644ed06cf970744d88076a9283518/Src/PerformanceCollector/Perf.Shared/QuickPulseTelemetryProcessor.cs#L184-L187

I would love to hear @Mikhail-Pranovich explanation of this finding.
If this is the only major cause for concern among the team, could this change be removed?

[TimothyMothra]

@SergeyKanzhelev Would you approve of this proposal?
Alex said we must have your signoff to take this change into the stable sdk.

[Mikhail-msft]

@MS-TimothyMothra, I suggest very careful architectural review for such a change. The reason I didn't merge this change in, is due to the fact that it spot fixes the performance bottleneck, rather than fixing the rootcause problem - the sampling needs to happen much early in the pipeline, comparing to current design. @Dmitry-Matveev was doing performance investigations, please sync with him if he has better solutions to this performance issue.

[Dmitry-Matveev]

We may be able to sample early in .NET Core SDK in the near future, however, there is no certainty in the early sampling approach for ASP.NET SDK currently. Most of my ongoing performance investigations are in ASP.NET Core SDK repo, so they won't interact with this particular change.

The change in the question is indeed a behavior change that can only be shipped by default with major version update. However, there is a possibility of shipping it behind the feature flag if absolutely necessary, thus making it opt-in scenario, where by default the experience does not change and any code leveraging the old sequencing of the property assignments will still continue to work for all the customers. Extension would need to enable this flag if extension is to consume this version from Nuget.

Regarding the upcoming updates regarding perf or ASP.NET SDK specifically, I'd touch base with @cijothomas.

[SergeyKanzhelev]

Even though I agree that sampler may need to happen earlier in a better world - I think this fix (which will only change behavior for auto-enabled apps) is perfecty justifiable.

BTW, for the easly sampling - in Open Census we use URL to get the span.Name and then use span.Name to pass to the sampler. Is there understanding why obtaining an URL is that expensive?

[github-actions]

This issue is stale because it has been open 300 days with no activity. Remove stale label or comment or this will be closed in 7 days.