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

Logging is not consistent #1157

Open
LadyCailinBot opened this issue Nov 8, 2017 · 1 comment
Open

Logging is not consistent #1157

LadyCailinBot opened this issue Nov 8, 2017 · 1 comment
Labels
feature request New functionality that doesn't exist yet

Comments

@LadyCailinBot
Copy link

CMDHELPER-3163 - Reported by PseudoKnight

We need to do a pass over the logging in CH to make it more consistent and work on various platforms correctly. (some platforms show [CommandHelper] twice in some locations, other times not at all) This means we need to use the plugin logger if available, and make it accessible from a singular location like Static. I propose Static.getLogger(). I'm not sure how CHLog would fit into this, though, as that seems to be more for warnings/errors. Maybe it should be adapted instead? We need to figure out what we want before fixing this.

@LadyCailinBot
Copy link
Author

LadyCailinBot commented Nov 8, 2017

Comment by LadyCailin

CHLog is meant for logging to files, however, we can create a new logging mechanism which is meant for logging to screen and to file, and integrate that with CHLog, then change all uses of CHLog to the new mechanism. Anything that is logged to file should also be logged to screen, I think. The new mechanism would look something like this:

class ItsALoggerButWeNeedABetterName {
    /**
     * This will log the item to screen only if Verbose logging is enabled
     * Target can be null for all items.
     */
    void verboseLogToScreen(String message, Target t);
    void debugLogToScreen(String message, Target t);
    void infoLogToScreen(String message, Target t);
    void etcLogToScreen(String message, Target t);

    /**
     * This will log the item to screen and using CHLog, only if logging is enabled.
     */
    void debugLog(CHLog.Tag tag, String message, Target t);
    void etcLog(CHLog.Tag tag, String message, Target t);
}

CHLog.Tag should probably be changed to an interface, and we allow extensions to @api tag these interfaces, and then we load them. The problem (perhaps) with this is that CHLog uses these tags to generate the config file, which users can then use to configure what levels are in fact logged. The preferences class should be just fine with upgrading preferences if extensions, etc, add new interfaces, so that should not be a problem.

interface LogTag {
    String tagName();
    String documentation();
    com.laytonsmith.core.LogLevel getDefaultLoggingLevel();
}

Then extensions etc that add new tags would do

@api
public enum ExtensionLogTags implements LogTag {
    MY_TAG("my_tag", "What it do yo", LogLevel.ERROR),
    MY_OTHER_TAG("my_other_tag", "What it do yo", LogLevel.INFO);
    /* boilerplate */
}

Alternatively, maybe we just want to combine the two mechanisms, and only have a single logging facility, which logs both to screen and to file, using the CHLog mechanism.

Actually, now that I think about it more, I think it probably makes sense to adapt CHLog instead, and just make Tags extendable, as shown above. (The enum can stay for convenience for core modules, but it too would implement the common interface.)

Also, let's rename it MSLog.

Perhaps we create a new function in MS which is able to re-work the to screen log levels (and hell, file logging too). So, at start up, the screen logging table just is copied from the config, but a call to set_screen_log_level('TAG', 'VERBOSE') would allow a runtime change to the screen logging.

Once we have this design solidified, we can also add a mechanism to change the underlying logging code, so that if a particular backend implementation needs to do the logging differently (bukkit vs spigot vs whatever), then you can install a new underlying mechanism that actually does the appropriate screen logging, whatever that may be. (Default would be to just System.out/err.println, etc.) And that would make it possible to change the underlying mechanism without having to change the code that is using the logger, just need to run a function at startup.

Hopefully this log of my thoughts was verbose enough :D

@LadyCailin LadyCailin added the feature request New functionality that doesn't exist yet label Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New functionality that doesn't exist yet
Projects
None yet
Development

No branches or pull requests

2 participants