Skip to content
This repository has been archived by the owner on Dec 8, 2018. It is now read-only.

Adding telemetry publish for unhandled exceptions to developer exception page #185

Merged
merged 1 commit into from
Oct 5, 2015

Conversation

JunTaoLuo
Copy link
Contributor

@dnfclas
Copy link

dnfclas commented Oct 2, 2015

Hi @JunTaoLuo, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

Assert.NotNull(listener.DiagnosticUnhandledException?.HttpContext);
Assert.NotNull(listener.DiagnosticUnhandledException?.Exception);
Assert.Null(listener.HostingUnhandledException?.HttpContext);
Assert.Null(listener.HostingUnhandledException?.Exception);
Copy link
Member

Choose a reason for hiding this comment

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

Assert NotNull HostingEndRequest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm guessing I should Assert NotNull HostingBeginRequest as well?

Copy link
Member

Choose a reason for hiding this comment

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

shrug that's out of scope of this change.

@Tratcher
Copy link
Member

Tratcher commented Oct 2, 2015

@avanderhoorn @rynowak

@Tratcher
Copy link
Member

Tratcher commented Oct 2, 2015

Include the bug number in the commit message.

@JunTaoLuo
Copy link
Contributor Author

@Tratcher I will add the bug number once I squash all commits

@avanderhoorn
Copy link
Member

When is this event triggered as apposed to "Microsoft.AspNet.Hosting.UnhandledException" here.

@JunTaoLuo
Copy link
Contributor Author

@avanderhoorn When the developer exception page successfully displays. Since the exception will be suppressed, we want to publish it to telemetry source as diagnostic unhandled exception. If for other reasons displaying the exception fails, the exception will be bubbled up to hosting and will trigger the telemetry publish there.

@avanderhoorn
Copy link
Member

Great, so in the case that this event happens, we wont get the hosting one.

@JunTaoLuo
Copy link
Contributor Author

@avanderhoorn right. In fact, without this change, the hosting one will never trigger any ways since the exception is not rethrown.

@avanderhoorn
Copy link
Member

Makes sense. Thanks! :shipit:

@JunTaoLuo
Copy link
Contributor Author

@Tratcher I was asking for that in my previous comment in the issue #180 . That error is technically handled and I wasn't sure if that should also publish to telemetry.

@avanderhoorn
Copy link
Member

For our part we would like to know about that exception too.

@JunTaoLuo
Copy link
Contributor Author

@avanderhoorn cool. I added the telemetry publish for exception handler as well. It's going to publish to Microsoft.AspNet.Diagnostics.HandledException just to differentiate from the unhandled exception from developer exception page, is that good?

@avanderhoorn
Copy link
Member

Sounds great!

On Friday, October 2, 2015, John Luo [email protected] wrote:

@avanderhoorn https://github.com/avanderhoorn cool. I added the
telemetry publish for exception handler as well. It's going to publish to
Microsoft.AspNet.Diagnostics.HandledException just to differentiate from
the unhandled exception from developer exception page, is that good?


Reply to this email directly or view it on GitHub
#185 (comment).

@JunTaoLuo JunTaoLuo force-pushed the johluo/adding-telemetry-to-exception-page branch from bd3cb1c to 7b9cfac Compare October 5, 2015 17:14
@JunTaoLuo
Copy link
Contributor Author

I'm going to take that as a :shipit: All tests passed so I'll merge to dev.

@JunTaoLuo JunTaoLuo merged commit 7b9cfac into dev Oct 5, 2015
@JunTaoLuo JunTaoLuo deleted the johluo/adding-telemetry-to-exception-page branch October 5, 2015 17:37
@avanderhoorn
Copy link
Member

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants