openframeworks / openFrameworks

openFrameworks is a community-developed cross platform toolkit for creative coding in C++.
http://openframeworks.cc
Other
9.89k stars 2.55k forks source link

ofSetLogLevel(module, level) adds to map but dosen't remove #1033

Open danomatika opened 12 years ago

danomatika commented 12 years ago

I talked to Arturo about this, but I want to make sure it's known. When you call ofLogSetLogLevel with a module string, that string is added to a map used to check the module's log level ... but right now that string is never removed.

If someone creates a large amount module names the map could grow arbitrarily large over time. The thought so far is that no one would make enough module names for it to be a problem.

For instance, using the proposed ofThread rewrite, each thread creates a new module name when it sets itself to verbose and, in the case of spawning lots of verbose worker threads, the map will grow.

My proposed solution would be to remove the module name from the map when the level for that module is set back to OF_LOG_NOTICE and/or to the current log level.

damian0815 commented 12 years ago

how about just keeping an eye on the size of the map, and if it gets too big just arbitrarily drop something, adding a warning to the console eg

ofLogWarning( "module name map got too big: dropping custom log level for " + droppedModuleName + ", will now default to " + currentDefaultLogLevel );

bilderbuchi commented 12 years ago

I don't know, "arbitrarily" doesn't sound like a good solution to me. Dan's proposal sounds good, the only thing to think about is probably what will happen if the global and or module log level change again? Maybe it makes sense to add module lifetime functions (something like removeModule(string modulename)) to ofLog? Then, when a thread ends, it can purge the module name from the list itself. Does that sound feasible?

danomatika commented 12 years ago

I think the easiest solution without using explicit add/remove module functions is to assume all log modules begin at log level NOTICE. If a module's level is changed, then it's added to the map. Setting it back to NOTICE removes it from the map.

bilderbuchi commented 12 years ago

ok. forgive me if I got this wrong (headache hooray), but what happens in this case: ofSetLogLevel("module1",OF_LOG_VERBOSE); //module1 goes into map ofSetLogLevel("module1",OF_LOG_NOTICE); //module1 is removed from map, because it's the same as global level ofSetLogLevel(OF_LOG_WARNING); // what happens now? we have no entries in the map, so how to we create entries for the modules which have a level different from the global level?

danomatika commented 12 years ago

The code checks if the modules exists in the map (aka it has a explicit level set), if not then it filters the module based on the current global log level.

In your case, logging a notice level message for "module1" will not print.

That's how the code works now.

bilderbuchi commented 12 years ago

and that's actually not correct, right? 'cause it should print a module1 message at notice level, because that's the level module1 was set to.

damian0815 commented 12 years ago

bilderbuchi, ofSetLogLevel could be renamed to ofSetLogMinLevel because that's what it's actually doing. every time you call ofLog you have to pass a level, and the ofSetLogLevel for individual modules is filtering the level of only that module. so if I want to get verbose OSC logging (ie get everything) and only notice threads, i'd call ofSetLogLevel( "ofxOsc", OF_LOG_VERBOSE ); ofSetLogLevel( "ofxThread", OF_LOG_NOTICE ); then i'd be able to read all of the OF_LOG_VERBOSE messages that ofxOsc prints, and none of the OF_LOG_VERBOSE messages that ofxThread prints.

right Dan?

bilderbuchi commented 12 years ago

damian: yeah, I'm aware how it works, I just read Dan's message as describing behaviour that feels unintended to me. ("In your case, logging a notice level message for "module1" will not print.")

Anyway, I just did a short test, and this code

ofSetLogLevel("module1",OF_LOG_VERBOSE); ofLogVerbose("module1") << "module1 verbose"; ofSetLogLevel("module1",OF_LOG_NOTICE); ofLogVerbose("module1") << "module1 verbose2"; ofLogNotice("module1") << "module1 notice"; ofSetLogLevel(OF_LOG_WARNING); ofLogNotice("module1") << "module1 notice2";

gives this output, as expected:

module1: OF_LOG_VERBOSE: module1 verbose module1: OF_LOG_NOTICE: module1 notice module1: OF_LOG_NOTICE: module1 notice2

so everything works as expected in current develop (except for the problem described in the OP, of course) sorry, multi-line code format seems to not work in comments.

elliotwoods commented 12 years ago

i think the question is, is the idea to set log levels per instance of something? (therefore each instance should have a local instance of something that is deleted when the instance is deleted. of course that might be a bit too OO for general oF use, but could perhaps be introduced for your case)

or are log levels set per definition of something (which discounts your 'per thread' log level problem)?

my current feeling is that log levels are to be set per definition

danomatika commented 12 years ago

If by "definition" you mean a unique string, then yes, they are set by definition.

This isn't a problem for ofThread since I use the unique thread id for the module name "Thread #".

elliotwoods commented 12 years ago

by definition i mean by object definition, i.e. generally a class name or an extension name. i'd say that each instance of 1 class (e.g. a threaded class) would be 1 instance

i'm just wondering whether 'Thread #' is really a 'module' or an instance of a module ofLogLevels for instances of modules is certainly an interesting feature, but i personally think that it should be differentiated from tracking the log level of static definitions of 'modules'