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

Differential loglevel #1678

Open
wants to merge 2 commits into
base: dev-3.x
Choose a base branch
from

Conversation

andrewnicols
Copy link
Contributor

This commit adds differential logging support such that different log
sources can have independent logLevels specified. This allows warnings to
be shown from noisy modules, whilst getting debugging from the item you are
actively developing.

I've also taken the liberty of splitting out the log tests from the rest of the tests after I got fed up of waiting for other tests to run.

I've also, also tidied up the console overrides to reduce code duplication, and to make sure that we actually reset the console afterwards.

Here's a sample YUI_config to demonstrate how this can work:

YUI_config = {
    // The default logLevel will be warn:
    logLevel: 'warn',
    logInclude: {
        // Reduce the logging threshold to debug for event-delegate:
        'event-delegate': 'debug',

        // Reduce the logging threshold to info for event-logged-at-info:
        'event-logged-at-info': 'info',

        // Reduce the logging threshold to Y.config.logLevel for event-logged-at-true:
        'event-logged-at-true': true,

        // Do not log anything for 'event-not-logged':
        'event-not-logged': false,

        // Do not log anything for everything else:
        '*': false
    }
};

Technically speaking, the * key on logExclude is not required, but I have included it for completeness.

As a note, this could be easily extended to support regular expressions, but I feel that this would probably be adding too much of an overhead at this stage

This commit adds differential logging support such that different log
sources can have independent logLevels specified. This allows warnings to
be shown from noisy modules, whilst getting debugging from the item you are
actively developing.
@andrewnicols
Copy link
Contributor Author

Passes browser tests on:

  • Chrome Mac
  • Chrome Windows
  • Chrome Canary Mac
  • IE 11 Windows
  • Safari Mac
  • Safari iOS 7.0
  • Firefox Mac
  • Firefox Windows
2237 yui3/src:differential_loglevel> yeti yui/tests/unit/index-logging.html
  Agent connected: Chrome (33.0.1750.117) / Mac OS from 127.0.0.1
  Agent connected: Internet Explorer (11.0) / Windows 8.1 from 10.211.55.3
  Agent connected: Firefox (27.0) / Mac OS from 127.0.0.1
  Agent connected: Chrome (35.0.1870.0) / Mac OS from 127.0.0.1
  Agent connected: Chrome (33.0.1750.117) / Windows 8.1 from 10.211.55.3
  Agent connected: Safari (7.0.2) / Mac OS from 127.0.0.1
  Agent connected: Safari (7.0) / iOS 7.0.3 from 192.168.100.108
  Agent connected: Firefox (26.0) / Windows 8.1 from 10.211.55.3
✓ Testing started on Chrome (33.0.1750.117) / Mac OS, Internet Explorer (11.0) / Windows 8.1, Firefox (27.0) / Mac OS, Chrome (35.0.1870.0) / Mac OS, Chrome (33.0.1750.117) / Windows 8.1, Safari (7.0.2) / Mac OS, Safari (7.0) / iOS 7.0.3, Firefox (26.0) / Windows 8.1
✓ Agent completed: Safari (7.0) / iOS 7.0.3
✓ Agent completed: Safari (7.0.2) / Mac OS
✓ Agent completed: Chrome (35.0.1870.0) / Mac OS
✓ Agent completed: Chrome (33.0.1750.117) / Mac OS
✓ Agent completed: Firefox (27.0) / Mac OS
✓ Agent completed: Firefox (26.0) / Windows 8.1
✓ Agent completed: Chrome (33.0.1750.117) / Windows 8.1
✓ Agent completed: Internet Explorer (11.0) / Windows 8.1
✓ 88 tests passed! (11 seconds)

@andrewnicols
Copy link
Contributor Author

Seeing these failures when I run yeti with phantom - will have to look into it more later as I'm leaving now. If anyone has any ideas why only phantom on node 0.10 is failing, I'm all ears!

@caridy
Copy link
Member

caridy commented Mar 4, 2014

Ok, here is what I think: I will like to reduce the complexity of Y.log, and add the proper hooks so other YUI modules can potentially hook into that process to do fancy things. I will be reluctant to add more and more complexity that target certain edge cases.

@andrewnicols
Copy link
Contributor Author

What did you have in mind @caridy?
I did look at whether it was possible to hook in using Y.config.logFn and do some stuff, but it's called too late :\

@caridy
Copy link
Member

caridy commented Mar 6, 2014

Simply to remove all the logLevel logic, and all Y.log features from the library, and let people to use a gallery module or a custom module to tap into the proper hooks to provide all these features that you want to add.

@andrewnicols
Copy link
Contributor Author

To me, the notion of removing Y.log seems like you're cutting off your nose to spite your face.
The logging is an incredibly useful, and powerful feature of YUI. The suggestion in this issue is to make it more powerful and give it some more of the features that other languages have expected for a long time - things like log::log4perl, log4j, and others for example have had this kind of differential logLevel for a very long time.

I really don't think that Gallery modules are the way forward here (for many reasons which I can detail if you wish), and moving everything to a custom module would be tricksy with regards deprecation and backwards compatibility. Additionally, if it were a custom module presumably the calls would not be removed by Shifter without a customised Shifter configuration for every build out there.

Writing a custom module which could supplement, or override Y.log is a possibility, but to me this really should be core functionality.

@caridy
Copy link
Member

caridy commented Mar 6, 2014

clarifying:

  • I suggested removing features that we implement today in Y.log, and keep Y.log() as a simple function doing almost nothing but calling console[level]().
  • Y.log() can be overwritten by devs, or you could do AOP when on development to have more control (filters, logLevel, etc).
  • We are actively evaluating how to reduce the size of the library, and one of things on the table is to align with other projects where log messages are not stripped out from the production code. This will remove the necessity of produce 4 files per module when running the build. To do that we will have to keep Y.log() code in the code we push to CDN.

@andrewnicols
Copy link
Contributor Author

Can I ask where these changes have been discussed? I've not seen anything in IRC, and although I don't watch every round table, I can't recall it being mentioned, and nor is it discussed in the blog.

If this is the case, we can go down the route of overwriting Y.log (or doing some form of AOP), but is there a sensible way of doing this with current Loader configuration? I'm not aware of anything which would do this reliably at present, but there are many parts of the loader which are under-documented.

@andrewnicols
Copy link
Contributor Author

Okay, so by using Y.config.core, you can replace the list of modules to either replace yui-log, or add a module directly after it. This module already has to have already been included as the loader won't include it for you.

@caridy
Copy link
Member

caridy commented Mar 6, 2014

@andrewnicols that's part of the work we have to do. @ekashida is working on the proper hooks that will allow you to patch things in YUI before any module gets evaluated, etc. Discussion about this topics are coming very soon!

@andrewnicols
Copy link
Contributor Author

Thanks @caridy,

I've managed to implement this in the mean time using Y.config.core and including a new logging module immediately after the standard yui-log. Only a PoC within Moodle but it does demonstrate the potential for such a solution.

Still don't entirely agree that we should remove logging entirely, but I can see the benefit of doing it this way for those with very obscure requirements, but I do disagree that this issue fits into that category.

Cheers,

Andrew

@caridy
Copy link
Member

caridy commented Mar 6, 2014

Yes, it is definitely doable. This is the code that I will like to see going away:
https://github.com/yui/yui3/blob/master/src/yui/js/yui-log.js#L38-L101

Reducing it to something like (plus a short-circuit to exit when logs are not needed):
https://github.com/yui/yui3/blob/master/src/yui/js/yui-log.js#L38-L101

And anything else, should be just an external module, maybe from bower, who knows!

The main situation is that Y.log tries to publish every event, and that will not work if we decide to keep those in production code due to performance reasons.

@andrewnicols
Copy link
Contributor Author

Did you mean to post the same URL there twice? ;) I assume it's mean to be L73-L85?

Please don't force people to use Bower - as I mentioned in my Roundtable talk last week, our build processes are very different to yours and our usage of YUI different also. Relying on this kind of process will force people in a particular direction which may not be suitable to their project at all.

@caridy
Copy link
Member

caridy commented Mar 7, 2014

good catch! yes, L73 to L85 (sort of).

we are not going to force people to use a particular tool. In fact I think that most people will use the default implementation, and will create their custom implementation for filters. this is the case of mojito for example, where there is a custom Y.log to deal with server and client, which also implements its own logLevels, etc.

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.

2 participants