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?
Conversation
…e completion and avoiding having to correct type channel name each time.
This is neat. I like the lightweight-ness of your implementation, that's quite in line with core UberLogger. One suggestion: change the "mute" flag into a filter level; thus, you can choose to mute just messages from an obscure system. I find that it is common to want to mute messages, but not warnings/errors. Example here. With filter level muting I'd vote for including this into core UberLogger. I've made a similar but more enterprise-y thing. It has an inspector + a source code generation step. The code generation allows for removal of the call sites for muted channels -> good for projects with lots of logging. It is a bit more clunky to use; the editor is still a bit clunky, and if the user generates source code for a bad configuration it is difficult to go back without reverting all changes to the configuration asset. I think what you have made is more suitable for core UberLogger. |
…annel entirely) and added tests to ensure filters are working.
Added flags to allow you to filter out messages now. You can now filter out normal .Log(...) calls (for example) by using something like |
UberLoggerChannel.cs
Outdated
/// </summary> | ||
public class UberLoggerChannel | ||
{ | ||
private string _channelName; |
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.
UberLoggerChannel.cs
Outdated
None = 0, | ||
HideLogs = 1, | ||
HideWarnings = 2, | ||
HideErrors = 4 |
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.
Would prefer 1<<1, 1<<2, 1<<3, etc.
UberLoggerChannel.cs
Outdated
[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 comment
The reason will be displayed to describe this comment to others. Learn more.
Could just be != 0 (and same for below).
UberLoggerChannel.cs
Outdated
public enum Filters | ||
{ | ||
None = 0, | ||
HideLogs = 1, |
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.
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
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.
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:
- Move the channel concept of UberLoggerChannel into the core API. Deprecate the old
UberDebug.LogChannel(string channel, ...)
submission points and replace them withUberDebug.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).
- 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.
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.
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.
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.
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 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!
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.
Also very nice! Again, happy to take with small changes. Thanks!
* Use << to shift bits instead of literal values * Remove 'Hide' prefix from filters that cause calls to be ignored * Compare with '== 0' instead of ' != Filters.XXX'
Added a utility class that wraps calls to Log*Channel(...) so that you can get code completion and don't have to worry about typing the channel name correctly each time.
My use case looks like this:
Then usage looks like:
And you get compile-time support (code completion, error checking, find-references, etc..). This is really useful when you want to standardize the usage of channels across a team because you don't have to remember capitalization and channel names.
Also you can easily turn on/off these channels without sprinkling your code with
if (muteMyChannel) { Debug.Log("...."); }
checks:For example: