-
Notifications
You must be signed in to change notification settings - Fork 60
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
base: master
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; | ||
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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
This will break existing code that uses
This will not break existing code that uses I prefer the first option. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
} | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
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.