Support nested C# exceptions #58

eadred · 2019-08-09T18:21:39Z

Inner exceptions
Exceptions thrown from asynchronous code

* Inner exceptions * Aggregate exceptions (from wait operations on failed tasks) * Exceptions thrown from asynchronous code

igorpeshansky · 2019-08-09T18:48:37Z

@eadred Thank you for the contribution!

This plugin is designed to work with Stackdriver Error Reporting, which should recognize the resulting multiline message as a stack trace. I'm verifying this with the team right now whether the backend would recognize a nested C# exception if the stack trace were joined into a single log entry. If the backend won't recognize these, there's no point in handling them in the plugin, and we should file an Error Reporting feature request.

igorpeshansky · 2019-08-09T19:46:58Z

I've confirmed with the Error Reporting team that all 3 new exceptions were picked up by Error Reporting, but maybe not in the form you'd expect. For example, for the CSHARP_AGGREGATE_EXC value from the test, only the content between the arrows was captured.

For the record, a self-service test can be done by running:

echo "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<---
" | \
gcloud beta error-reporting events report \
  --service manual \
  --service-version test-version \
  --project=$(gcloud config get-value project) \
  --message-file=/dev/stdin

(see the gcloud docs).

If the ingestion results don't look ok, let's open a feature request against Error Reporting before proceeding with this PR. For the values that work correctly, we can start reviewing those portions of the PR.

igorpeshansky

Some preliminary comments.

igorpeshansky · 2019-08-09T19:52:45Z

lib/fluent/plugin/exception_detector.rb

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


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?

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

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.

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.

igorpeshansky · 2019-08-09T19:54:45Z

test/plugin/test_exception_detector.rb

  at System.Threading.Thread.StartInternal () [0x00000] in <filename unknown>:0
 END

+  CSHARP_NESTED_EXC = <<END.freeze


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?

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.

qingling128 · 2019-08-09T20:56:53Z

lib/fluent/plugin/exception_detector.rb

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


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

qingling128 · 2019-08-09T20:58:05Z

test/plugin/test_exception_detector.rb

  at System.Threading.Thread.StartInternal () [0x00000] in <filename unknown>:0
 END

+  CSHARP_NESTED_EXC = <<END.freeze


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.

eadred · 2019-08-09T22:56:43Z

lib/fluent/plugin/exception_detector.rb

+      # C# exception from async code
+      rule([:java_after_exception, :java],
+           %r{^---\sEnd\sof\sstack\strace\sfrom\sprevious\s
+           location\swhere\sexception\swas\sthrown\s---}x,


The linter doesn't like this ('Use // around regular expression.'), but without it I end up with a line that is too long for the linter. Have you got any suggestions about how to split the regex across multiple lines (Ruby isn't my language)?

I could perhaps define a '+' operator for Regexp, as suggested here.

Try the /x modifier, as suggested here.

Thanks. Done.

igorpeshansky

Please don't force-push changes once the PR is being reviewed. Force-pushes make it impossible to retain conversation threads. It's ok to have multiple commits in a PR — we'll squash-and-merge them anyway.

Can you restore the previous commit chain (git reflog is your friend), add the latest changes in top of that, and re-push?

igorpeshansky · 2019-08-09T23:33:34Z

lib/fluent/plugin/exception_detector.rb

+      # C# exception from async code
+      rule([:java_after_exception, :java],
+           %r{^---\sEnd\sof\sstack\strace\sfrom\sprevious\s
+           location\swhere\sexception\swas\sthrown\s---}x,


Try the /x modifier, as suggested here.

* Aligning method params properly * Attempting to put long regex on multi-lines

* Don't use %r{} around regex

Updated the state machine to allow multiple current states, to cope with the case where it is ambiguous if the start of an exception is Java or C#

eadred · 2019-08-12T07:10:32Z

Sorry about the force push. I've now restored the original commit as it was at the time of the first review (1df893b).

I've now managed to get the C# rules into a separate set of rules (in the most recent commit a991faa). As I mentioned in an earlier comment, I've now had to cope with the fact that it is ambiguous as far as the rules are concerned whether you are seeing a C# or a Java exception. To implement this I've had to support the state machine having more than one possible current state. For example, it might be in either the :java_start_exception or the :csharp_start_exception state when it see a line like "foo.bar.baz.Exception: Something went wrong."

Unfortunately these changes have made the bench tests around 3 to 4 times slower because the ExceptionDetector.transition method can't exit early once it has found a match. I'm not sure there's much I can do about this - is it preferable to have the cleaner separation of rules with worse performance, or to go back to the mixing of C# and Java-like rules?

eadred · 2019-08-12T12:18:10Z

I checked how the new C# exceptions looked in error reporting, and it seems it is only the aggregate exception that doesn't get picked up properly. It is actually the other two types of exception I am more interested in getting handled properly, so if it meant this PR could get merged faster I could drop the logic for handling the aggregate exception. What do you think?

qingling128 · 2019-08-12T21:01:43Z

@eadred - Thanks for refactoring the code. Now I see why it was challenging to separate csharp and java states. We care a lot of performance, so 3X - 4X processing time is sufficient to make an exception for this case. We probably want to revert the a991faa commit.

As to the aggregate exception, let's drop that part (please open a feature request with Error Reporting backend if needed) so that we can get this PR in.

This was causing too much of a slowdown

Handles multiple inner exceptions

This is pending https://issuetracker.google.com/issues/139292720

eadred · 2019-08-13T08:11:36Z

Thanks for that.

I've now reverted a991faa (as 7fa27c0) and also removed the handling for aggregate exceptions (58889d3).

I've raised an issue with the Error Reporting team for handling AggregateExceptions - https://issuetracker.google.com/issues/139292720.

PS. I realised that my handling of aggregate exceptions wasn't dealing with the case where there are multiple inner exceptions, so I did put in a fix for that (6ea0cb7). Although this has all been reverted anyway, I may want to re-add these changes in a future PR.

qingling128

LGTM

qingling128 · 2019-08-13T16:38:30Z

Thanks @eadred! This looks good to me. Seems like there are still pending comments from @igorpeshansky. He is on vacation and will be back on Monday (Aug 19). Is this something that can wait or is it urgent?

eadred · 2019-08-13T17:39:37Z

Thanks @qingling128. No problem if this is delayed - it isn't urgent.

qingling128 · 2019-08-13T18:50:55Z

Cool, thanks. Will wait for @igorpeshansky to take a second look once he's back.

igorpeshansky · 2019-08-19T17:37:21Z

lib/fluent/plugin/exception_detector.rb

      rule(:java_after_exception, /^[\r\n]*$/, :java_after_exception),
      rule([:java_after_exception, :java], /^[\t ]+(?:eval )?at /, :java),
+
+      # C# nested exception


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

igorpeshansky · 2019-08-19T17:37:35Z

lib/fluent/plugin/exception_detector.rb

+           /^[\t ]+--- End of inner exception stack trace ---/,
+           :java),
+
+      # C# exception from async code


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

igorpeshansky · 2019-08-19T17:38:53Z

lib/fluent/plugin/exception_detector.rb

+
+      # C# exception from async code
+      rule([:java_after_exception, :java],
+           /^---\sEnd\sof\sstack\strace\sfrom\sprevious\s


These \s all over the place are hard to read. They seem to be imposed by the use of /x. If so, we may be better off turning the x option on locally, e.g.:

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

Alternatively, you could use \ instead of \s, as we do in the first PHP rule below, though I find that less readable.

Done as per the former suggestion.

igorpeshansky · 2019-08-19T17:39:22Z

lib/fluent/plugin/exception_detector.rb

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


Does this need a trailing $?

igorpeshansky · 2019-08-19T17:39:28Z

lib/fluent/plugin/exception_detector.rb

+      # C# exception from async code
+      rule([:java_after_exception, :java],
+           /^---\sEnd\sof\sstack\strace\sfrom\sprevious\s
+           location\swhere\sexception\swas\sthrown\s---/x,


Does this need a trailing $?

igorpeshansky · 2019-08-19T17:59:06Z

lib/fluent/plugin/exception_detector.rb

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


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.

igorpeshansky

LGTM

xflag · 2019-10-31T08:18:37Z

@igorpeshansky Could you release a new version for this issue?
Now I need this enhancement,I use .net core running in docker swarm,then collect log from docker logging driver to fluentd which add this plugin for multiline error logs.

qingling128 · 2019-11-01T20:39:04Z

I'll work with our oncall @davidbtucker to do a release.

qingling128 · 2019-11-01T20:39:24Z

Sent #65.

qingling128 · 2019-11-04T17:15:43Z

Done: https://github.com/GoogleCloudPlatform/fluent-plugin-detect-exceptions/releases/tag/0.0.13

igorpeshansky self-requested a review August 9, 2019 18:45

Support nested C# exceptions

1df893b

* Inner exceptions * Aggregate exceptions (from wait operations on failed tasks) * Exceptions thrown from asynchronous code

igorpeshansky added the enhancement label Aug 9, 2019

eadred force-pushed the csharp-nested-exceptions branch from ef8ce9d to 1df893b Compare August 9, 2019 18:48

igorpeshansky requested a review from qingling128 August 9, 2019 19:54

igorpeshansky suggested changes Aug 9, 2019

View reviewed changes

qingling128 requested changes Aug 9, 2019

View reviewed changes

eadred force-pushed the csharp-nested-exceptions branch from 1df893b to 800b860 Compare August 9, 2019 21:39

eadred commented Aug 9, 2019

View reviewed changes

igorpeshansky suggested changes Aug 9, 2019

View reviewed changes

eadred added 3 commits August 10, 2019 17:24

Linting changes

980d6a2

* Aligning method params properly * Attempting to put long regex on multi-lines

Linting changes

fc9cdd7

* Don't use %r{} around regex

Create distinct rules/states for C# exceptions

a991faa

Updated the state machine to allow multiple current states, to cope with the case where it is ambiguous if the start of an exception is Java or C#

eadred force-pushed the csharp-nested-exceptions branch from 800b860 to a991faa Compare August 12, 2019 06:54

jkohen assigned qingling128 Aug 12, 2019

eadred added 3 commits August 13, 2019 07:30

Reverting separate rule list for C#

7fa27c0

This was causing too much of a slowdown

Handling for complex .Net aggregate exception

6ea0cb7

Handles multiple inner exceptions

Removed handling for .Net aggregate exceptions

58889d3

This is pending https://issuetracker.google.com/issues/139292720

qingling128 approved these changes Aug 13, 2019

View reviewed changes

igorpeshansky suggested changes Aug 19, 2019

View reviewed changes

PR review changes

5b7dd34

igorpeshansky approved these changes Aug 20, 2019

View reviewed changes

igorpeshansky added the cla: yes label Aug 20, 2019

igorpeshansky merged commit bf6ef35 into GoogleCloudPlatform:master Aug 20, 2019

Support nested C# exceptions #58

Support nested C# exceptions #58

Uh oh!

Conversation

eadred commented Aug 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igorpeshansky commented Aug 9, 2019

Uh oh!

igorpeshansky commented Aug 9, 2019

Uh oh!

igorpeshansky 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!

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!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eadred commented Aug 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eadred commented Aug 12, 2019

Uh oh!

qingling128 commented Aug 12, 2019

Uh oh!

eadred commented Aug 13, 2019

Uh oh!

qingling128 left a comment

Choose a reason for hiding this comment

Uh oh!

qingling128 commented Aug 13, 2019

Uh oh!

eadred commented Aug 13, 2019

Uh oh!

qingling128 commented Aug 13, 2019

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

eadred commented Aug 9, 2019 •

edited

Loading

eadred commented Aug 12, 2019 •

edited

Loading

xflag commented Oct 31, 2019 •

edited

Loading