Optimize Exception Handling and Log

[taooceros]

1. Introduce Demystifier library to make stacktrace more readable. https://github.com/benaadams/Ben.Demystifier
2. Make QueryResult Method async void to allow exception thrown, which will trigger the report window on runtime.
3. Use ExceptionDispatchInfo in Log.Exception to preserve stacktrace
4. Make NLog log in async, and make Log.Exception not implemented synchronously.
5. Use NLog's exception layout renderer to render exception.
6. Use NLog's OutputDebugStringTarget to print to debug window
7. Change layout to ${logger}->${time}|${level}|${message} (need discussion) (Exception layout is similar but too more new line is added before and after Exception block)
   • PluginManager->21:40:16.0845|Debug|"QueryForPluginAsync"|"Cost for System Commands <14ms>"|
8. Replace all "prefix|UnPrefix" message to call overload that allow a className and methodName
9. Use Conditional("DEBUG") for Debug method in logger.

The Exception block has been simplified to only Exception.ToString, because that will include message, Exception Type, and Stacktrace. I don't believe extra HResult or TargetSite will help us debugging.

TODO:

• Unify Program Logger

Some Discussion

• Do we want every exception thrown by plugin to report in a report window? (if not, do we want a message to notice user?)

Future (need discussion)

• Make Logging system not static class but with per instance logger (which allows NLog to catch callsite)

[jjw24]

Do we want every exception thrown by plugin to report in a report window? (if not, do we want a message to notice user?)

For now maybe report window as long as the link to report it is to the plugin's website not flow launcher unless they are default plugins. Reason is i would rather plugin devs be aware of the issue so they could fix it.

[jjw24]

Make Logging system not static class but with per instance logger (which allows NLog to catch callsite)

what do you mean callsite?

[taooceros]

Make Logging system not static class but with per instance logger (which allows NLog to catch callsite)

what do you mean callsite?

If the log is logged directly. NLog allows to record caller information (like PluginManager.QueryAsync), so we won't need to record that separately.

[jjw24]

Make Logging system not static class but with per instance logger (which allows NLog to catch callsite)

what do you mean callsite?

If the log is logged directly. NLog allows to record caller information (like PluginManager.QueryAsync), so we won't need to record that separately.

Perfect, as long as it records the calling method

[taooceros]

Perfect, as long as it records the calling method

Yeah, that's why currently we cannot do that, because we Log via another method which will change the callsite.

Maybe we can simply expose a readonly logger for people to use.

[jjw24]

👍 sounds good

[taooceros]

Hmmm NLog's document suggests that callsite can be expensive (include a stacktrace tracking), but they are improving it in NLog 5.0. Maybe currently it would be better to still record caller by ourselves.

[jjw24]

Damn, ok sounds good

[taooceros]

Another question is, should we removed the manual message. "prefix|unprefix", while prefix means classname and methodname, and direct all usage to Log.${Level}(message, classname, methodname[CallerMemberName])?

[jjw24]

yes 👍

[taooceros]

Done

[pc223]

Do we want every exception thrown by plugin to report in a report window? (if not, do we want a message to notice user?)

For now maybe report window as long as the link to report it is to the plugin's website not flow launcher unless they are default plugins. Reason is i would rather plugin devs be aware of the issue so they could fix it.

I vote yes 👍 on this. Make sure users know something break and where to get help is good.

Should the report window have logs.txt file content and have a button to copy the logs too?

I think it's better that Flow exposes the logs file a bit more. I see a lot of users have hard time to find the logs file, especially with non-portable mode, the logs file path is really...long and obscure.

I'm thinking of something like this 🤓

Better error reporting is really useful when we introduce plugin-making for users that not professional dev.

[jjw24]

Please resolve conflict.

A fair bit of changes here, what have you tested so far?

Do we want this in 1.8.0?

I think it's better that Flow exposes the logs file a bit more. I see a lot of users have hard time to find the logs file, especially with non-portable mode, the logs file path is really...long and obscure.

@pc223 1.8.0 has command to open log file location directly, let me know if it's sufficient

[taooceros]

Do we want this in 1.8.0?

At least part of it, including @pc223 's point.

[taooceros]

I haven't notice this one when reviewing the last change. Why do we need to include another JsonStorage?

[jjw24]

That one is because in the old infra dll it looks for JsonStrorage class, the incorrectly spelled JsonStorage

[taooceros]

That one is because in the old infra dll it looks for JsonStrorage class, the incorrectly spelled JsonStorage

Oh, got you. I didn't notice the typo.

[jjw24]

is this pr ready for review?

does it need to be in 1.8.0(a lot of changes across several plugins from a glance)?

[taooceros]

is this pr ready for review?

does it need to be in 1.8.0(a lot of changes across several plugins from a glance)?

The major changes across plugins are from the api revise. You can take a glance on it, or if that's too much for review, let me pick a few things that worth to be included in 1.8.0.

[jjw24]

is this pr ready for review?
does it need to be in 1.8.0(a lot of changes across several plugins from a glance)?

The major changes across plugins are from the api revise. You can take a glance on it, or if that's too much for review, let me pick a few things that worth to be included in 1.8.0.

Yeah I think could you please pull out a few that is worth to be included into a separate pr and let me know what you have tested with them so I don't double up with my own testing

[taooceros]

@jjw24 Do we want to include the layout change in 1.8.0?

[jjw24]

Layout change? What do you mean

[jjw24]

The log message layout?

[taooceros]

The log message layout?

Yes

[jjw24]

Yeah sure

[jjw24]

@taooceros could you also merge dev and resolve conflict please

[taooceros]

@taooceros could you also merge dev and resolve conflict please

Sure, will do tonight

[pc223]

Do we want every exception thrown by plugin to report in a report window? (if not, do we want a message to notice user?)

@taooceros @jjw24 I think we should try to push this report-window-feature to release soon, because it affects adoption of the new Python plugins template, plugin-dev should aware of exception like this (or any invalid request/response RPC)

18:14:57.0716+07:00 - ERROR - JsonRPCPlugin.ExecuteAsync - Traceback (most recent call last):
  File "C:\Users\user1\AppData\Roaming\FlowLauncher\Plugins\Search-MDI-2.1.1\main.py", line 13, in <module>
    from plugin.main import MDI
ModuleNotFoundError: No module named 'plugin'

I caught this exception while trying to install the Search-MDI plugin, it indicates the main.py missing these lines. This should not break silently 😎

import sys,os
parent_folder_path = os.path.abspath(os.path.dirname(__file__))
sys.path.append(parent_folder_path)
sys.path.append(os.path.join(parent_folder_path, 'lib'))
sys.path.append(os.path.join(parent_folder_path, 'plugin'))

[jjw24]

Do we want every exception thrown by plugin to report in a report window? (if not, do we want a message to notice user?)

@taooceros @jjw24 I think we should try to push this report-window-feature to release soon, because it affects adoption of the new Python plugins template, plugin-dev should aware of exception like this (or any invalid request/response RPC)

18:14:57.0716+07:00 - ERROR - JsonRPCPlugin.ExecuteAsync - Traceback (most recent call last):
  File "C:\Users\user1\AppData\Roaming\FlowLauncher\Plugins\Search-MDI-2.1.1\main.py", line 13, in <module>
    from plugin.main import MDI
ModuleNotFoundError: No module named 'plugin'

I caught this exception while trying to install the Search-MDI plugin, it indicates the main.py missing these lines. This should not break silently 😎

import sys,os
parent_folder_path = os.path.abspath(os.path.dirname(__file__))
sys.path.append(parent_folder_path)
sys.path.append(os.path.join(parent_folder_path, 'lib'))
sys.path.append(os.path.join(parent_folder_path, 'plugin'))

yep makes sense to me.

Could you @taooceros merge dev again please

[taooceros]

The feature has been merged. Jsonrpc exception is suppressed for now, but will be exposed in #633

[taooceros]

The only thing left is the exception api refractor. Probably do a clean one will be easier than merging it, but anyway, I will take a try later.

[jjw24]

@taooceros when you get a chance, could you merge dev in, i gave it a shot but still had many differences i am unsure of, so i reverted back.

[taooceros]

@taooceros when you get a chance, could you merge dev in, i gave it a shot but still had many differences i am unsure of, so i reverted back.

Sure

[taooceros]

It's too hard to merge....I will refactor the rest of the part once I am free