Skip to content

Support nested C# exceptions #58

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Aug 20, 2019
Merged

Support nested C# exceptions #58

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1df893b
Support nested C# exceptions
eadred Aug 9, 2019
980d6a2
Linting changes
eadred Aug 10, 2019
fc9cdd7
Linting changes
eadred Aug 10, 2019
a991faa
Create distinct rules/states for C# exceptions
eadred Aug 12, 2019
7fa27c0
Reverting separate rule list for C#
eadred Aug 13, 2019
6ea0cb7
Handling for complex .Net aggregate exception
eadred Aug 13, 2019
58889d3
Removed handling for .Net aggregate exceptions
eadred Aug 13, 2019
5b7dd34
PR review changes
eadred Aug 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions lib/fluent/plugin/exception_detector.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,22 @@ def self.supported
:java_start_exception),
rule(:java_after_exception, /^[\r\n]*$/, :java_after_exception),
rule([:java_after_exception, :java], /^[\t ]+(?:eval )?at /, :java),

# C# nested exception
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this comment to the line right before the regexp, and add the trailing ".".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

rule([:java_after_exception, :java],
/^[\t ]+--- End of inner exception stack trace ---/,
:java),

# C# aggregate exception
rule([:java_after_exception, :java],
/^---> \(Inner Exception #[0-9]+\)/,
Copy link
Contributor

Choose a reason for hiding this comment

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

These look sufficiently different from Java that they merit a separate set of rules for C# (possibly sharing some of the Java rules programmatically, e.g., by copying the rules and doing symbol substitution). @qingling128, WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. At least we should separate the names (not having java_after_exception in a C# exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem I'm having implementing this is there's not a good way of distinguishing between what is a C# exception and what is a Java exception when first transitioning to a *_start_exception state. Essentially both have to look for Exception in the text.

Because of this, if the JAVA_RULES are added first to ALL_RULES before CSHARP_RULES, then C# exceptions go off down the Java state machine path, and therefore never encounter C# specific rules like the ones I'm trying to implement. Likewise, if CSHARP_RULES were to appear before JAVA_RULES then Java exceptions get picked up by the C# state machine.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see the problem here. The Java state machine could use a lot of improvement, but that's way beyond the scope of this PR, so what you have is fine.

:java),

# C# exception from async code
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this comment to the line right before the regexp, and add the trailing ".".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

rule([:java_after_exception, :java],
/^--- End of stack trace from previous location where exception was thrown ---/,
:java),

rule([:java_after_exception, :java], /^[\t ]*(?:Caused by|Suppressed):/,
:java_after_exception),
rule([:java_after_exception, :java],
Expand Down
50 changes: 50 additions & 0 deletions test/plugin/test_exception_detector.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,50 @@ class ExceptionDetectorTest < Test::Unit::TestCase
at System.Threading.Thread.StartInternal () [0x00000] in <filename unknown>:0
END

CSHARP_NESTED_EXC = <<END.freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be time to define an array of exceptions per language. This is a much larger refactoring than we should reasonably expect you to do in this PR, but if you're game, that's great. If not, we could try to do it ourselves, and then rebase your PR on top of the result. @qingling128, WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with merging this PR once it's approved and refactoring them into arrays per language later. This way we unblock the use case earlier.

System.InvalidOperationException: This is the outer exception ---> System.InvalidOperationException: This is the inner exception
at ExampleApp.NestedExceptionExample.LowestLevelMethod() in c:/ExampleApp/ExampleApp/NestedExceptionExample.cs:line 33
at ExampleApp.NestedExceptionExample.ThirdLevelMethod() in c:/ExampleApp/ExampleApp/NestedExceptionExample.cs:line 28
at ExampleApp.NestedExceptionExample.SecondLevelMethod() in c:/ExampleApp/ExampleApp/NestedExceptionExample.cs:line 18
--- End of inner exception stack trace ---
at ExampleApp.NestedExceptionExample.SecondLevelMethod() in c:/ExampleApp/ExampleApp/NestedExceptionExample.cs:line 22
at ExampleApp.NestedExceptionExample.TopLevelMethod() in c:/ExampleApp/ExampleApp/NestedExceptionExample.cs:line 11
at ExampleApp.Program.Main(String[] args) in c:/ExampleApp/ExampleApp/Program.cs:line 11
END

CSHARP_AGGREGATE_EXC = <<END.freeze
System.AggregateException: One or more errors occurred. (This is an exception) ---> System.InvalidOperationException: This is an exception
at ExampleApp.AsyncExceptionExample.LowestLevelMethod() in c:/ExampleApp/ExampleApp/AsyncExceptionExample.cs:line 28
at ExampleApp.AsyncExceptionExample.ThirdLevelMethod() in c:/ExampleApp/ExampleApp/AsyncExceptionExample.cs:line 23
at ExampleApp.AsyncExceptionExample.SecondLevelMethod() in c:/ExampleApp/ExampleApp/AsyncExceptionExample.cs:line 17
at ExampleApp.AsyncExceptionExample.TopLevelMethod() in c:/ExampleApp/ExampleApp/AsyncExceptionExample.cs:line 12
--- End of inner exception stack trace ---
at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
at System.Threading.Tasks.Task.Wait()
at ExampleApp.Program.Main(String[] args) in c:/ExampleApp/ExampleApp/Program.cs:line 12
---> (Inner Exception #0) System.InvalidOperationException: This is an exception
at ExampleApp.AsyncExceptionExample.LowestLevelMethod() in c:/ExampleApp/ExampleApp/AsyncExceptionExample.cs:line 28
at ExampleApp.AsyncExceptionExample.ThirdLevelMethod() in c:/ExampleApp/ExampleApp/AsyncExceptionExample.cs:line 23
at ExampleApp.AsyncExceptionExample.SecondLevelMethod() in c:/ExampleApp/ExampleApp/AsyncExceptionExample.cs:line 17
at ExampleApp.AsyncExceptionExample.TopLevelMethod() in c:/ExampleApp/ExampleApp/AsyncExceptionExample.cs:line 12<---
END

CSHARP_ASYNC_EXC = <<END.freeze
System.InvalidOperationException: This is an exception
at ExampleApp2.AsyncExceptionExample.LowestLevelMethod() in c:/ExampleApp/ExampleApp/AsyncExceptionExample.cs:line 36
at ExampleApp2.AsyncExceptionExample.<ThirdLevelMethod>d__2.MoveNext() in c:/ExampleApp/ExampleApp/AsyncExceptionExample.cs:line 31
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
at ExampleApp2.AsyncExceptionExample.<SecondLevelMethod>d__1.MoveNext() in c:/ExampleApp/ExampleApp/AsyncExceptionExample.cs:line 25
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
at ExampleApp2.AsyncExceptionExample.<TopLevelMethod>d__0.MoveNext() in c:/ExampleApp/ExampleApp/AsyncExceptionExample.cs:line 14
END

RUBY_EXC = <<END.freeze
NoMethodError (undefined method `resursivewordload' for #<BooksController:0x007f8dd9a0c738>):
app/controllers/books_controller.rb:69:in `recursivewordload'
Expand Down Expand Up @@ -570,6 +614,9 @@ def test_js

def test_csharp
check_exception(CSHARP_EXC, false)
check_exception(CSHARP_NESTED_EXC, false)
check_exception(CSHARP_AGGREGATE_EXC, false)
check_exception(CSHARP_ASYNC_EXC, false)
end

def test_python
Expand Down Expand Up @@ -627,6 +674,9 @@ def test_mixed_languages
check_exception(GO_ON_GAE_EXC, false)
check_exception(GO_SIGNAL_EXC, false)
check_exception(CSHARP_EXC, false)
check_exception(CSHARP_NESTED_EXC, false)
check_exception(CSHARP_AGGREGATE_EXC, false)
check_exception(CSHARP_ASYNC_EXC, false)
check_exception(V8_JS_EXC, false)
check_exception(RUBY_EXC, false)
check_exception(DART_ERR, false)
Expand Down