Mixedrealitytoolkit-unity: vNext Proposal: Implement specifying logging level

[davidkline-ms]

Overview

Per #3077, customers have a desire to configure the debug output that the toolkit components emit. In past experience, it has been helpful to be able to opt-in /out to specific classes of messages.

This task is to add a set of logging level flags and allow the app developer to specify the desired message types via the MixedRealityToolkit component. Other components can check the current logging level and determine whether or not to log.

This level will be configurable in the inspector and can be changed at runtime (another VERY handy option I have found).

Fair warning: this will impact a large number of files in a simple and predictable way.

Proposed flags:

• Informational (aka Debug.Log)
• Assert (Debug.Assert)
• Warning (Debug.LogWarning)
• Error (Debug.LogError)
• CriticalError (Debug.LogError with significant functionality impact, ex: specified system failed)

Requirements

• Application developer can customize the types of log messages to be emitted.
• Built-in MRTK components, including core and registered services honor the logging level
• Logging level can be changed (via script and the inspector) and be honored by MRTK components.

Acceptance Criteria

• [x] Logging level configurable through the MRTK configuration profile
• [x] Logging level can be changed at runtime
• [x] Supporting logging level is as easy as calling DebugUtilities.Log*
• [x] Multiple logging levels (MRTK and app specific) can be supported by passing a custom logging level to the logging methods
• [x] Toolkit component honors the logging level
• [ ] Existing services honor the logging level

[StephenHodgson]

I'm not keen on making our own Logging system, when this is something Unity already handles on its own.

Maybe we should be opening a feature request to Unity?

[SimonDarksideJ]

i can understand having a log filtering system to prevent us logging when users don't want to. But I don't think we should be changing how we log. At least not in this release.

We would need significant data from the uses of logging by users / consumers / etc before even considering something like this, else it has a severe risk of being over-engineered

[StephenHodgson]

Agreed, also this seems much more like a proposal than a task.

[davidkline-ms]

This is directly related to a customer issue / request. Having the ability to filter debug output is often key to being able to understand issues with client specific code.

[davidkline-ms]

i can understand having a log filtering system to prevent us logging when users don't want to. But I don't think we should be changing how we log. At least not in this release.

This is not changing how we log, it is adding to the configuration profile (default value is Everything) and adding, for example:

if ((LoggingLevel & LoggingLevels.CriticalError) != 0)
{
}

around the log calls.

[davidkline-ms]

For systems (and any other component that wishes to participate in the logging levels), the wrapper looks like

if ((MixedRealityToolkit.LoggingLevel & LoggingLevels.CriticalError) != 0)

[davidkline-ms]

Feedback from customers that have been spoken to has been positive wrt this approach.

[StephenHodgson]

We could possibly add extension methods that help specify logging levels as well.

[davidkline-ms]

We could possibly add extension methods that help specify logging levels as well.

We could. My current prototype wraps with the if statement. I can definitely investigate extension methods. I would expect a call to them to look something like:

Debug.LogWarning(MixedRealityToolkit.LoggingLevel, "Warning: Something unexpected occurred.");

Or alternatively:

Debug.LogMessage(Warning, MixedRealityToolkit.LoggingLevel, "A warning message.");

[davidkline-ms]

The method would check the type of message with the configured level and log as appropriate (using an if like the ones described above).

[StephenHodgson]

I'm down with:

Debug.LogWarning(MixedRealityToolkit.LoggingLevel, "Warning: Something unexpected occurred.");

[StephenHodgson]

Although, why even have the Logging Level?

That could just be wrapped up in the extension itself.

[davidkline-ms]

Although, why even have the Logging Level?

The logging level is to allow customers to configure based on their needs (ex: turn off all informational logs, or only show warnings) - the way my prototype works even allows it to be changed at runtime. In my past this type of functionality has been very helpful to identify specific application bugs.

[StephenHodgson]

I mean yeah, have a configurable logging level, but I meant only use it in the extension method.

Not pass it as a parameter.

[davidkline-ms]

I mean yeah, have a configurable logging level, but I meant only use it in the extension method.
Not pass it as a parameter.

Interesting thought. It just feels odd to have the extension method / class have a reference to the toolkit scene object.

[StephenHodgson]

have a reference to the toolkit scene object.

What scene reference? You lost me man. Likely the LoggingLevel will be a setting on a configuration profile.

If we add the logging level to every single debug statement in our library, then it also prevents people from consuming our services in a third party service locator pattern.

Although I'm pretty certain that we can't extend a method without additional parameters anyways, so the point might be moot.

[SimonDarksideJ]

On, let's draw a line under this as there seen too be more requirements around living that need to go in to a task and discuss.

This specific pr addresses the initial errors when a system is not configured and should be merged in as is.