Mikp/web/private perf build

microsoft / ApplicationInsights-dotnet-server

Microsoft Application Insights for .NET Web Applications

https://azure.microsoft.com/services/application-insights/

133 stars 67 forks source link

Mikp/web/private perf build #1174

Closed TimothyMothra closed 5 years ago

TimothyMothra commented 5 years ago

Fix Issue #1173

removes RequestTelemetry.Url from `RequestTrackingTelemetryModule.
removes RequestTelemetry.Source from `RequestTrackingTelemetryModule.
Adds new PostSamplingTelemetryProcessor to populate the previously removed properties. (Defered execution)
restores RequestTelemetry.Url to QuickPulseTelemetryProcessor

TimothyMothra commented 5 years ago

@Dmitry-Matveev can I get you to review this? If you're okay with these changes, I'll release alpha5 today

cijothomas commented 5 years ago

@MS-TimothyMothra Is this how its going to work:

We merge this changes in main branch and ship it.
The default behavior wont be affected for SDK customers.
In Code-Less attach, via config, PostSampling. etc will be enabled.

lmolkova commented 5 years ago

In the long term (second half of 2019) this problem is intended to be solved with sampling on Activity level: https://github.com/dotnet/corefx/issues/36349

We need to make sampling decision on request when we start processing it and set it on Acitivity.Current.Recording. Then we can always rely on Activity.Current.Recording when we need to set expensive properties.

In this case there is no need in extra flags and telemetry module, but we might need to expose sampling APIs in base SDK. Also it does not seem right to use HttpContext.Current in processors which make the whole approach error-prone and unreliable.

If this change is not time critical I propose to postpone it and make things better without introducing new things that also subject to break.

TimothyMothra commented 5 years ago

@cijothomas Yes, that is the plan. I already have a branch with the config required for codeless attach.

@lmolkova This is to fix a problem we're actively having. Codeless attach is using a forked version of the SDK which is creating a maintenance nightmare. We need these changes in the SDK and hidden behind a feature flag.

lmolkova commented 5 years ago

This is to fix a problem we're actively having. Codeless attach is using a forked version of the SDK which is creating a maintenance nightmare. We need these changes in the SDK and hidden behind a feature flag.

My point is, relying on HttpContext.Current surviving to a processor is not a good idea and I'm afraid we are moving nightmare inside the SDK and exposing feature flags/APIs that are not going to be used in the long term, but will have to maintain them forever. Have we evaluated options to avoid this?

TimothyMothra commented 5 years ago

Have we evaluated options to avoid this?

@lmolkova We're very open to suggestions, but I don't think waiting until the end of this year is good enough.

Yes, HttpContext.Current will be a problem if the processor is run on a different thread, but I've been assured that this doesn't happen in the SDK today.

TimothyMothra commented 5 years ago

Per offline conversation, I will work on these requested changes Friday.