Automates MVC and WebApi exception handling by lmolkova · Pull Request #847 · microsoft/ApplicationInsights-dotnet-server

lmolkova · 2018-03-07T19:02:46Z

This change automates MVC error handler and WebApi exception logger injection.

Now, we ask customers to do it manually as per this article: https://docs.microsoft.com/en-us/azure/application-insights/app-insights-asp-net-exceptions. VS 'Add AplicationInsights' wizard also adds MVC filter.

It allows tracking semi-unhandled exceptions - for requests resulted in 500 in the typical and default production case when custom errors handling was enabled.

This helps to onboard SnapshotDebugger customers ensuring important exceptions are tracked and in general, improves error detection for all AppInsights ASP.NET customers hosting their apps on IIS>

This change supports only WebAPI 2 and MVC4 (Microsoft.AspNet.Mvc/WebApi nuget version 5).
Earlier versions are not supported since ASP.NET nuget v5 was released in 2013 and is first to target .NET 4.5. (ASP.NET v4 targets .NET 4.0)

This change is done through the reflection-based dynamic code generation to prevent AI Web SDK dependency on MVC and WebApi dlls. If such dependency is introduced, it creates difficulties with binding redirects that are unresolvable with SDK light-up.

This reflection-based solution implements fail-fast approach - if some type is not resolved, it silently stops preventing further injection.

All type generation and injection are done at app startup (when ExceptionTrackingTelemetryModule is initialized) therefore performance impact on requests after service warms up remains intact.
Generation and injection time remains within reasonable few milliseconds range.

I ran Unit Tests locally.

For significant contributions please make sure you have completed the following items:

Changes in public surface reviewed
the new property is introduced:
ExceptionTrackingTelemetryModule EnableMvcAndWebApiExceptionAutoTracking to disable injection through the config
Design discussion issue - discussed over emails/meetings with @SergeyKanzhelev and @zakimaksyutov
CHANGELOG.md updated with one line description of the fix, and a link to the original issue.

lmolkova · 2018-03-07T19:10:38Z

Src/Web/Web.Net45/Implementation/HttpRequestExtensions.cs

@@ -18,7 +18,7 @@ public static HttpCookie UnvalidatedGetCookie(this HttpRequest httpRequest, stri
        public static string UnvalidatedGetHeader(this HttpRequest httpRequest, string headerName)
        {
            string value = httpRequest.Unvalidated.Headers[headerName];


we should always expect a header value to be null

EnforceMaxLength has Debug.Assert that value is not null preventing to run tests locally in debug.

Instead, I can remove Debug.Assert from the EnforceMaxLength, however I don;t know what was the motivation putting it there in the first place.

@MS-TimothyMothra,
It seems you've done this change. could you please advise what is the best approach here?

If it's causing problems, please remove it.
I added it because it seemed like it was the convention.
I have a backlog item to remove it myself and create actual unit tests for that Common project.

will do.

I also wonder what is the need for enforcing max length on the incoming side? If request made it to the service, why would we trim it? Where 1024 came from?

This risk was identified from SDL review.
Request headers are tracked in a dictionary and an attacker could spam an application with very large header values and force an out of memory exception.
Sergey gave me the 1024 value. That should be exaggeratedly larger than we need, but still small enough to protect ourselves from an attack.

pharring · 2018-03-07T19:13:49Z

Src/Web/Web.Shared.Net/ExceptionTrackingTelemetryModule.cs

+            {
+                if (disposing)
+                {
+                    this.isInitialized = false;


Why reset this? Do you want to allow re-initialization after Dispose? (Perhaps Initialize should check disposed and throw an ObjectDisposedException

it's for testing only - as we need to initialize module more than once in the same process.

It would be better to expose internal classes via InternalVisibleTo than complicating the public API

while I can test underlying ExceptionHandlersInjector, testing it does not guarantee that it's ever called. So, in any case, I would need Dispose/clear logic on the ExceptionTrackingTelemetryModule to verify it calls ExceptionHandlersInjector.

I can substitute public dispose with internal isInitialized field.
I believe almost all modules are disposables and it fits into the model better than internal fields accessible by tests.

Thoughts?

lmolkova · 2018-03-07T19:14:17Z

Src/Web/Web.Shared.Net/ExceptionTrackingTelemetryModule.cs

+            if (this.EnableMvcAndWebApiExceptionAutoTracking)
+            {
+                if (!this.isInitialized)
+                {


some other telemetry modules have this logic to ensure they are initialized just once as it seems it's not guaranteed by the base SDK.

Even if the injection is attempted more than once, it could be a bit expensive to run it more than once anyway, so I'd prefer it to be done just once.

Sure, but are we forgetting to set isInitialized to true somewhere?

nice catch, thanks!

lmolkova · 2018-03-07T19:14:32Z

Src/Web/Web.Shared.Net/RequestTrackingTelemetryModule.cs

-                    rootRequestId = StringUtilities.EnforceMaxLength(rootRequestId, InjectionGuardConstants.RequestHeaderMaxLength);
                    if (rootRequestId != null)
                    {
+                        rootRequestId = StringUtilities.EnforceMaxLength(rootRequestId, InjectionGuardConstants.RequestHeaderMaxLength);


same as before:

we should always expect a header value to be null

EnforceMaxLength has Debug.Assert that value is not null preventing to run tests locally in debug.

Instead, I can remove Debug.Assert from the EnforceMaxLength, however I don;t know what was the motivation putting it there in the first place.

@MS-TimothyMothra,
It seems you've done this change. could you please advise what is the best approach here?

Please remove the Debug.Assert. (see comment above)

pharring · 2018-03-07T19:18:21Z

Src/Web/Web.Shared.Net/Implementation/ExceptionHandlersInjector.cs

+    using Microsoft.ApplicationInsights.DataContracts;
+    using Microsoft.ApplicationInsights.Extensibility.Implementation.Tracing;
+
+    internal sealed class ExceptionHandlersInjector


Can it be static?

TimothyMothra · 2018-03-07T21:31:50Z

CHANGELOG.md


 ## Version 2.6.0-beta2
+- [Implement unhandled exception auto-tracking (500 requests) for MVC 5 and WebAPI 2 applications] (https://github.com/Microsoft/ApplicationInsights-dotnet-server/pull/847)
 - [Added a max length restriction to values passed in through requests.](https://github.com/Microsoft/ApplicationInsights-dotnet-server/pull/810)


Please add your comment to "2.6.0-beta3". Beta 2 is being released this week. I can explain offline.

SergeyKanzhelev · 2018-03-08T00:02:00Z

Src/Web/Web.Shared.Net/Implementation/WebEventSource.cs

+        [Event(43,
+            Keywords = Keywords.Diagnostics,
+            Message = "{0} Injection failed. Error message: {1}",
+            Level = EventLevel.Verbose)]


I'd suggest informational or warning. It may help diagnose issues without going verbose

SergeyKanzhelev · 2018-03-08T00:03:09Z

Src/Web/Web.Shared.Net/Implementation/ExceptionHandlersInjector.cs

+            //        telemetryClient.TrackException(new ExceptionTelemetry(context.Exception));
+            //    base.OnLog(context);
+            // }
+            // public override void OnLog(ExceptionLoggerContext context)


so IL injection is better than reading load pre-compiled assembly from resources because we need to support multiple versions of MVC and will need binding redirect?

Precompiled assemble must still be compiled with some specific version on MVC/WebApi.
To avoid binding redirects and assembly resolution issues, we considered an option here to use AppDomain.CurrentDomain.AssemblyResolve handler to manually resolve assembly versions when it's not possible automatically.
we preferred IL injection approach because

such event fires for dlls MVC/WebApi depend on. And while public API surface is stable, particular dll dependencies might change: i.e. new dependency is introduced in MVC v6.

it is more invasive and may potentially conflict with other components doing the same.

while IL injection is harder to maintain, the thing we are doing here is small and simple and does not seem to be changed in future.

If we'd need to write more complicated logic, i'd say we should change it AppDomain.CurrentDomain.AssemblyResolve approach.

Before this moment my definition of "small and simple" was a bit different =)

:) I mean the code we are emitting is small and simple - just a few lines of code. I would not try to emit AppInisghts SDK

SergeyKanzhelev

See comments. Look OK overall

Dmitry-Matveev · 2018-03-08T00:29:20Z

Src/Web/Web.Shared.Net.Tests/Helpers/HttpModuleHelper.cs

        }

-        public static HttpContext GetFakeHttpContext(IDictionary<string, string> headers = null, Func<string> remoteAddr = null)
+        public static HttpContext GetFakeHttpContext(IDictionary<string, string> headers = null, Func<string> remoteAddr = null, bool isCustomErrorEnabled = false)


isCustomErrorEnabled parameter does not seem to be used within the method - is it only required to satisfy the method signature as it's called out of our control somewhere?

nice catch, parameter should be removed

Dmitry-Matveev · 2018-03-08T02:56:30Z

Src/Web/Web.Shared.Net/Implementation/ExceptionHandlersInjector.cs

+                var exceptionTelemetryCtor = GetConstructorOrFail(typeof(ExceptionTelemetry), new[] { typeof(Exception) });
+
+                var assemblyName = new AssemblyName(AssemblyName);
+                var assemblyBuilder = AppDomain.CurrentDomain.DefineDynamicAssembly(assemblyName, AssemblyBuilderAccess.Run);


Does app need to run in full trust for those calls, do we need any SecurityPermissionFlags?

excellent question!

I followed this guide to ensure this code requires only SecurityPermissionFlags.Execute (the minimum possible).
So the answer is no, we don't need any flags.

I've been trying to run any ASP.NET 5 app (empty one, no custom code or AI involved at all) with the trust level less than Full with no luck - it fails before any user code is executed. So I assume ASP.NET no longer supports other modes than Fill trust. My security and trust related knowledge in .NET is quite limited, so I can be wrong here.
Do you know something about it?

zakimaksyutov · 2018-03-09T05:09:41Z

What will happen to customers who followed instructions? Will we collect exceptions twice?

And if app catches and traces exceptions but doesn't rethrow them (responds with 500 directly) then this approach will not catch exceptions, correct?

lmolkova · 2018-03-09T18:29:50Z

What will happen to customers who followed instructions? Will we collect exceptions twice?

Yes, exceptions will be tracked twice.
We should also change documentation and mention that customers should remove their manual instrumentation.
This problem exists with any other auto-collection things we do - we always expect some customers to track it manually. In the typical application, 500 responses should be rare and additional exception telemetry should have a negligible effect on the telemetry rate.
We could consider marking exception object as tracked to avoid duplication. This would require a change in the base SDK though.

And if app catches and traces exceptions but doesn't rethrow them (responds with 500 directly) then this approach will not catch exceptions, correct?

Right, if app constructs 500 response itself, there may be no exception at all or it could be handled in the controller. In such case, we won't track it. It is the best practice to respond with 5XX for handled internal server errors and helps devops to classify known issues and distinguish them from unknown (500). We can just hope that if they manually catch exceptions they also manually track them.
All this info should be added (if not already there) in the https://docs.microsoft.com/en-us/azure/application-insights/app-insights-asp-net-exceptions article.

I'll update the article if you are ok with above.

lmolkova · 2018-03-09T18:30:26Z

@zakimaksyutov please see my comments above

lmolkova · 2018-03-12T19:06:13Z

Docs PR: https://github.com/MicrosoftDocs/azure-docs-pr/pull/35107

lmolkova · 2018-03-12T20:02:03Z

@zakimaksyutov @SergeyKanzhelev @MS-TimothyMothra

If there are no more concerns, can we merge it?

SergeyKanzhelev · 2018-03-12T20:15:52Z

No concerns. You can merge.

lmolkova requested a review from SergeyKanzhelev March 7, 2018 19:07

lmolkova self-assigned this Mar 7, 2018

lmolkova requested review from Dmitry-Matveev, TimothyMothra and cijothomas March 7, 2018 19:07

lmolkova commented Mar 7, 2018

View reviewed changes

pharring reviewed Mar 7, 2018

View reviewed changes

lmolkova commented Mar 7, 2018

View reviewed changes

pharring reviewed Mar 7, 2018

View reviewed changes

Liudmila Molkova added 4 commits March 7, 2018 11:21

Automated MVC WebApi exception handling

e249d04

changelog

f0a1d0a

minor fix

3099657

review comments, change severity of not-supported-version trace

619cbbf

lmolkova force-pushed the lmolkova/ExceptionHandling branch from fbfa592 to 619cbbf Compare March 7, 2018 19:54

TimothyMothra reviewed Mar 7, 2018

View reviewed changes

Liudmila Molkova added 2 commits March 7, 2018 14:20

remove debug.assert change changelog

6d0671a

up

7178800

SergeyKanzhelev reviewed Mar 8, 2018

View reviewed changes

SergeyKanzhelev approved these changes Mar 8, 2018

View reviewed changes

Dmitry-Matveev approved these changes Mar 8, 2018

View reviewed changes

review

f241ebe

Merge branch 'develop' into lmolkova/ExceptionHandling

d7e9ae7

lmolkova merged commit 33696c0 into develop Mar 12, 2018

Conversation

lmolkova commented Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmolkova Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zakimaksyutov commented Mar 9, 2018

Uh oh!

lmolkova commented Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lmolkova commented Mar 9, 2018

Uh oh!

lmolkova commented Mar 12, 2018

Uh oh!

lmolkova commented Mar 12, 2018

Uh oh!

SergeyKanzhelev commented Mar 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lmolkova commented Mar 7, 2018 •

edited

Loading

lmolkova Mar 7, 2018 •

edited

Loading

lmolkova commented Mar 9, 2018 •

edited

Loading