Skip to content
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.

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

Utility class for logging to channel #43

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 43 additions & 0 deletions Examples/TestUberLogger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

public class TestUberLogger : MonoBehaviour
{
public static readonly UberLoggerChannel TestChannelWrapper = new UberLoggerChannel("WrapperChannel");

Thread TestThread;
// Use this for initialization
void Start ()
Expand Down Expand Up @@ -71,6 +73,47 @@ public void DoTest()
UberDebug.LogErrorChannel("Test", "ULogErrorChannel with param {0}", "Test");
UberDebug.LogErrorChannel(gameObject, "Test", "ULogErrorChannel with GameObject");
UberDebug.LogErrorChannel(gameObject, "Test", "ULogErrorChannel with GameObject and param {0}", "Test");

// Will output all messages in test function.
RunChannelWrapperTests();

// Will hide .Log(...) calls in test function.
TestChannelWrapper.Filter = UberLoggerChannel.Filters.HideLogs;
RunChannelWrapperTests();

// Will hide .LogWarning(...) calls in test function.
TestChannelWrapper.Filter = UberLoggerChannel.Filters.HideWarnings;
RunChannelWrapperTests();

// Will hide .LogError(...) calls in test function.
TestChannelWrapper.Filter = UberLoggerChannel.Filters.HideErrors;
RunChannelWrapperTests();

// Will hide .Log(...) and LogWarning(...) calls in test function.
TestChannelWrapper.Filter = UberLoggerChannel.Filters.HideLogs | UberLoggerChannel.Filters.HideWarnings;
RunChannelWrapperTests();
}

private void RunChannelWrapperTests()
{
Debug.Log("Running Channel Wrapper Tests...");

TestChannelWrapper.Log("Wrapped Channel");
TestChannelWrapper.Log("Wrapped Channel with param {0}", "Test");
TestChannelWrapper.Log(gameObject, "Wrapped Channel with GameObject");
TestChannelWrapper.Log(gameObject, "Wrapped Channel with GameObject and param {0}", "Test");

TestChannelWrapper.LogWarning("Wrapped Channel Warning");
TestChannelWrapper.LogWarning("Wrapped Channel Warning with param {0}", "Test");
TestChannelWrapper.LogWarning(gameObject, "Wrapped Channel Warning with GameObject");
TestChannelWrapper.LogWarning(gameObject, "Wrapped Channel Warning with GameObject and param {0}", "Test");

TestChannelWrapper.LogError("Wrapped Channel Error");
TestChannelWrapper.LogError("Wrapped Channel Error with param {0}", "Test");
TestChannelWrapper.LogError(gameObject, "Wrapped Channel Error with GameObject");
TestChannelWrapper.LogError(gameObject, "Wrapped Channel Error with GameObject and param {0}", "Test");

Debug.Log("... Done Running Channel Wrapper Tests.");
}

// Update is called once per frame
Expand Down
89 changes: 89 additions & 0 deletions UberLoggerChannel.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
using System.Collections;
using System.Collections.Generic;
using UberLogger;
using UnityEngine;

/// <summary>
/// Wraps access to a named channel in a class so that it can be used without having to
/// type the channel name each time to enforcing channel name checking at compile-time.
/// </summary>
public class UberLoggerChannel
{
private string _channelName;
Copy link
Owner

Choose a reason for hiding this comment

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

Slightly to my regret, the coding standard would be ChannelName for a private member variable that isn't shadowed by a property.

public UberLoggerChannel(string channelName)
{
_channelName = channelName;
Filter = Filters.None;
}

/// <summary>
/// Filters for preventing display of certain message types.
/// </summary>
[System.Flags]
public enum Filters
{
None = 0,
HideLogs = 1,
Copy link
Owner

Choose a reason for hiding this comment

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

Not 100% sure what I think of this. As @Kalmalyzer says, this is useful for performance purposes, and thus a very practical addition. At the same time:

  • Filtering at this level means the messages never reach the core logging system and are lost forever.
  • It's functionality that doesn't exist in the raw channel logging API - you only get to filter if you use the nice new wrappers.

At the very least, I think we should drop the 'Hide' prefix (since they don't 'hide' them, they drop them entirely), e.g Filters.Logs, Filters.Warnings etc

Copy link
Contributor

Choose a reason for hiding this comment

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

About the filtering...

I see UberLogger as working like this:

A number of submission points -> core message bus -> a number of output modules

Filtering should be either in the submission point(s) or in the core message bus, not in the output modules.

UberLoggerChannel adds a wrapper around an existing submission point, and also introduces per-channel muting at the new submission point.

There is an API in the core today for filtering channels; the IFilter interface does this. Attaching the UberLogger-FilterChannels extension provides per-channel filtering.

If it is important to move filtering to the core, then I suggest we take one of two paths:

  1. Move the channel concept of UberLoggerChannel into the core API. Deprecate the old
    UberDebug.LogChannel(string channel, ...) submission points and replace them with UberDebug.LogChannel(UberLoggerChannel channel, ...) submission points. Pass around UberLoggerChannel refs instead of channel string names within the core. Remove the IFilter extension point. Introduce logic within UberLogger such that filter settings on an UberLoggerChannel are applied at the time where the IFilter was used. Introduce an extra API method to control filter settings for "no channel".

This will break existing code that uses UberDebug.LogChannel(). It makes channel usage a slight bit more complicated, in that the user must declare channels up-front. However, it will lead to a more type-safe design (the primary type of a channel is no longer a string, it is an UberLoggerChannel).

  1. Continue to treat channels as strings in the core API. Remove the IFilter extension point, and replace it with an API that allows controlling filter settings for channels. (This would sort-of be like taking UberLogger-FilterChannels and modifying & internalizing that functionality.) Modify UberLoggerChannel to communicate the channel's filter settings into the core filter settings API at the time that the UberLoggerChannel object is constructed.

This will not break existing code that uses UberDebug.LogChannel(). However, we do not get the benefits of the type safety.

I prefer the first option.

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 not sure replacing the string channel name with an object provides any more utility than simply making a bunch of static string's and using those instead of string literals. Assuming you have to name each channel anyways (for proper display), it seems like making these core objects is just introducing more complexity with very little gain. Having the channel name be a string literal is pretty understandable for most people and makes it clear that sending two messages to a channel with the same name is always going to be output the same.

If you were to switch to an 'object-based' channel system (vs. 'string-based') under the hood what do we do when someone new's up a channel object with the same name as an existing one? Do they go to the same place? Are their filter settings the same? As a naive user, I might expect them to be treated the same, but without checking docs I wouldn't be sure.

Also, using strings allows you to refer to channels dynamically in a data-driven system without having to worry about passing a channel object around or maintaining a central mapping of 'channel name' -> 'channel object'. For example, if my game had N cars driving around, I might want to log each cars decision into a separate channel so I can easily tell what logs were relevant to each car. If there's a channel object required, I have to share that object between all places I want to log (which requires more bookkeeping). If it's just a string it's easier to generate that without having to modify your data structures or method params to pass around the logging objects.

Using strings internally (i.e. keeping the existing LogChannel(...) API) seems to be a good mix of simplicity and flexibility. Internalizing the FilterChannels (or some similar core mechanism) and making the UberLoggerChannel.Filters some syntactic sugar for modifying that seems like a better approach to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good points. I'm okay with either option. @bbbscarter are you on board with moving filtering into the core? If so I (or @astruyk if you are eager) could build that in a separate PR.

Copy link
Owner

Choose a reason for hiding this comment

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

Good points all round.

On the philosophical point of where to put a filter - perhaps controversially, my 'purist' preference is for it to be in the outputs, rather than the core or the inputs. This is just because the outputs are the only place you can put a filter without the information being lost forever - e.g. with the in-editor log window and a nice UI you can change your filtering decisions on the live results, throughout the entire run. There's nothing more frustrating than successfully reproing a bug and then finding you weren't capturing the right data.

At the same time, this is less useful for file logging, logging does have a memory and performance cost, and there's already an API for filtering. So yeah, putting channel filtering into the core API sounds good. :)

I'd like to keep strings for the core API for channels, just because it's simple.

Core filter API thought - we could just add an optional 'channelName' parameter to the existing AddFilter function, and if it's set maintain a new dict of <channelName, List>? Should be a small change. Add in a new SeverityFilter IFilter implementation and we have what we need. (We might need RemoveFilter as well).

If we did this, would we even need syntactic sugar for UberLoggerChannel filters? e.g. logChannel.Filters = new SeverityFilter(Severity.Warning) isn't overly burdensome, and is more flexible.

Regardless, this all sounds fab, and is a great discussion. Thanks for your time on this!

HideWarnings = 2,
HideErrors = 4
Copy link
Owner

Choose a reason for hiding this comment

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

Would prefer 1<<1, 1<<2, 1<<3, etc.

}

/// <summary>
/// Gets or sets the current filters being applied to this channel. Messages that match the specified set of flags will be ignored.
/// </summary>
public Filters Filter { get; set; }

[StackTraceIgnore]
public void Log(string message, params object[] par)
{
if ((Filter & Filters.HideLogs) != Filters.HideLogs)
{
UberDebug.LogChannel(_channelName, message, par);
}
}

[StackTraceIgnore]
public void Log(Object context, string message, params object[] par)
{
if ((Filter & Filters.HideLogs) != Filters.HideLogs)
Copy link
Owner

Choose a reason for hiding this comment

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

Could just be != 0 (and same for below).

{
UberDebug.LogChannel(context, _channelName, message, par);
}
}

[StackTraceIgnore]
public void LogWarning(string message, params object[] par)
{
if ((Filter & Filters.HideWarnings) != Filters.HideWarnings)
{
UberDebug.LogWarningChannel(_channelName, message, par);
}
}

[StackTraceIgnore]
public void LogWarning(Object context, string message, params object[] par)
{
if ((Filter & Filters.HideWarnings) != Filters.HideWarnings)
{
UberDebug.LogWarningChannel(context, _channelName, message, par);
}
}

[StackTraceIgnore]
public void LogError(string message, params object[] par)
{
if ((Filter & Filters.HideErrors) != Filters.HideErrors)
{
UberDebug.LogErrorChannel(_channelName, message, par);
}
}

[StackTraceIgnore]
public void LogError(Object context, string message, params object[] par)
{
if ((Filter & Filters.HideErrors) != Filters.HideErrors)
{
UberDebug.LogErrorChannel(context, _channelName, message, par);
}
}
}
13 changes: 13 additions & 0 deletions UberLoggerChannel.cs.meta

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.