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

Conversation

astruyk
Copy link
Contributor

@astruyk astruyk commented Feb 26, 2018

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:

  1. Make a static class with a number of UberLoggerChannel as members:
public static class Logger {
    public static readonly UberLoggerChannel Core = new UberLoggerChannel("Core");
    public static readonly UberLoggerChannel Achievements = new UberLoggerChannel("Achievements ");
    // .... More channels
}

Then usage looks like:

Logger.Core.Log("Foo");
Logger.Achievements.LogWarning("Bar");

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:

public static class Logger {
    public static readonly UberLoggerChannel ObscureSystem = new UberLoggerChannel("ObscureSystem");
    // ... Other channels

    static Logger() {
        ObscureSystem.Mute = true;
    }
};

…e completion and avoiding having to correct type channel name each time.
@Kalmalyzer
Copy link
Contributor

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.
@astruyk
Copy link
Contributor Author

astruyk commented Feb 27, 2018

Added flags to allow you to filter out messages now. You can now filter out normal .Log(...) calls (for example) by using something like ObscureSystem.Filter = UberLoggerChannel.Filters.HideLogs; or ObscureSystem.Filter = UberLoggerChannel.Filters.HideLogs | UberLoggerChannel.Filters.HideWarnings;

/// </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.

None = 0,
HideLogs = 1,
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.

[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).

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!

Copy link
Owner

@bbbscarter bbbscarter left a 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'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants