GNOME Bugzilla – Bug 627864
log: Revamp the log system
Last modified: 2010-09-03 08:28:26 UTC
Created attachment 168665 [details] [review] Revamp the log system Current log system suffers from a few drawbacks: * It uses Glib's default handler for all the domains, this handler should not be overriden by a library (imagine what can happen if every library depends on being able to override the default log handler), * You can't use the common practice to break on g_log() to trace where warnings come from because the decision of printing the message is taken in the handler itself, * Other gnome libraries tend to define a single glib log domain for the whole library. So, instead, use a GStreamer-like logging system, with "log domains" you have to declare and initialize. This patch tries to keep the original author's intent by keeping: * the glib's debug levels and using them in the final g_logv(), * the same decoding of the string given to grl_log_init(). It also modifies a bit when the GRL_LOG env variable is sampled. Instead of requiring the use to explictely call grl_log_init() to check the GRL_LOG behing the scene, grilo now gets this variable at startup and overrides the default verbosity level of log domains when they are created. grl_log_init() can still be used to set the verbosity, it has to be called after the log domain has been initialized which, for plugins, means after having loaded the plugin.
Created attachment 168666 [details] [review] update plugins to the new logging API This commit adapts the logging code to the new GRL_LOG_* defines and log domain code. See the corresponding commit in core for further details.
Created attachment 168667 [details] [review] remove grl_log_init from the tools grl-inspect: Now that we default to only print warnings, it's actually pretty sane to still show those warning that shows errors one should fix. grl-test-ui: The default verbosity of the test UI make seeing real warnings really hard. It's still possible to recover the verbose mode with GRL_LOG.
Just did a smoke test, and I've a few comments: * You need to add grl-log-priv.h into noinst_HEADERS list in src/Makefile.am * The registry check is broken given the new log subsystem (we need to ignore some warning messages) * Thus the make distcheck is broken
Created attachment 168761 [details] [review] Update the patch to core Updated the patch with fixes from the review in comment #3 * added grl-log-priv.h to the list of sources files * Fix make check by updating the exclusion rules for fatal warnings in src/tests/registry.c
Another idea that pops out of my mind, is about removing the _LOG_ namespace in the logging macros: GRL_DEBUG instead of GRL_LOG_DEBUG and so on... Juan, what do you thing? (Iago is on holidays right now) OTOH: This is just a personal concert: but I'm not sure if the whole gstreamer logging machinery is what grilo needs.
* I added _LOG_ to the GRL_LOG_ defines because GRL_ERROR was already defined as a GQuark for a GError, that said the GQuark can be renamed to have GRL_ERROR/GRL_DEBUG/etc free for use in the logging API * I want to underline that's not really suitable to override the default glib log handler :p. * In term of functionalities and API exported to the user, this is nothing compared what GStreamer actually provides (colored output, quite a few runtime API to access the categories and verbosity). It's basically what grilo has now but without using Glib's log domains * If this ever goes in, I would love to add another verbosity level called TRACE and some convenience macros to replace the g_debug ("grl_my_function_name"); grilo has everywhere.
(In reply to comment #6) > * I added _LOG_ to the GRL_LOG_ defines because GRL_ERROR was already defined > as a GQuark for a GError, that said the GQuark can be renamed to have > GRL_ERROR/GRL_DEBUG/etc free for use in the logging API Aha! Agree then. Maybe renaming GRL_ERROR would make sense. > * I want to underline that's not really suitable to override the default glib > log handler :p. Utterly agree. The bug is there, no question on that. > * In term of functionalities and API exported to the user, this is nothing > compared what GStreamer actually provides (colored output, quite a few runtime > API to access the categories and verbosity). It's basically what grilo has now > but without using Glib's log domains Fair observation. > * If this ever goes in, I would love to add another verbosity level called > TRACE and some convenience macros to replace the g_debug > ("grl_my_function_name"); grilo has everywhere. Yeah, and I would love to get rid of all of them
With no doubt having a powerful log system helps developers quite a lot. I've been suffering for this problem in other projects, and I wouldn't like Grilo with this problem. I think the proposed patches helps to improve logging mechanism in Grilo, and making it more "standard". Of course, it would be terrific to have Gstreamer logging system in Grilo. But it wouldn't be fair to compare, right now, Grilo with Gstreamer. They walked their own way to reach the system they have. But pretty sure we can steal some ideas from them ;)
(In reply to comment #7) > > * I want to underline that's not really suitable to override the default glib > > log handler :p. > > Utterly agree. The bug is there, no question on that. Moving this bug to NEW.
After the release, let's integrate these patches. First of all I don't like the GRL_LOG_ namespace, so I propose to rename the GRL_ERROR error domain to GRL_CORE_ERROR (yes, mimicking GStreamer again), so the logging macros could be GRL_ERROR/GRL_WARNING and so on. I'll cook a patch for the renaming, and aftewards rebase the logging patch. What do you thing? Is this a good plan for you?
(In reply to comment #10) > > I'll cook a patch for the renaming, and aftewards rebase the logging patch. Proposed patches for the GRL_ERROR error domain renaming: http://mail.gnome.org/archives/grilo-list/2010-September/msg00001.html http://mail.gnome.org/archives/grilo-list/2010-September/msg00002.html
Created attachment 169259 [details] [review] Revamp the log system - take 3 Renamed GRL_LOG_* to GRL_*, eg GRL_LOG_DEBUG to GRL_DEBUG. Note that GRL_ERROR used to be the global GQuark for Grilo's GError domain, but since bug #628506 the log domain should be renamed to GRL_CORE_ERROR as defined in the GError documentation http://library.gnome.org/devel/glib/unstable/glib-Error-Reporting.html
Created attachment 169260 [details] [review] update plugins to the new logging API - take 3 Renamed GRL_LOG_* to GRL_LOG. As the previous patch, it depends on the plugin patch in bug #628506 to be applied
Created attachment 169280 [details] [review] log: handle numeric level assignations Besides the symbolic level recognition, this patch also enable the numeric level setting. Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>
(In reply to comment #14) > Created an attachment (id=169280) [details] [review] > log: handle numeric level assignations > > Besides the symbolic level recognition, this patch also enable the numeric > level setting. > > Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com> Damien, are you agree with this change? With it, now the GRL_LOG envvar can be "*:5" or so. Also, I'd like to change GRL_LOG envvar for GRL_DEBUG what do you think?
I totally agree with both changes, I was actually going to propose the second one. With that Grilo is closer to what other libraries do (GST_DEBUG, CLUTTER_DEBUG, GTK_DEBUG, etc...). While at it, I'm not really happy with grl_log_init(), we might want a grl_log_configure_from_string() or just grl_log_configure() :p, patches follow.
Created attachment 169365 [details] [review] Rename the environment variable to GRL_DEBUG The log system uses an environment variable to override the verbosity of log domains. Let's use GRL_DEBUG to be closer to what other gnome libraries use (GST_DEBUG, GTK_DEBUG, CLUTTER_DEBUG).
Created attachment 169366 [details] [review] Remove the call to grl_log_init() in JS and vala tests Now that Grilo default to only print warning and error messages there's no need to call grl_log_init() in the JS and vala tests as they won't be as verbose as it used to be. Printing warning and error messages is really useful to find so programming mistakes and thus those messages should not be discarded in the tests. That is precisely the default behaviour of the log system, so there's no need to call grl_log_init().
Created attachment 169367 [details] [review] Rename grl_log_init() to grl_log_configure() You can configure one or several log domains at run time with this API, so it's not an initialization per se, it's closer to a configuration string one gives to the log system. Along with the renaming, properly document the format of the string used to configure log domains.
Created attachment 169368 [details] [review] tests: Adapt to the grl_log_init() renaming in core (patch to -plugins) grl_log_init() has been renamed to grl_log_configure() in master, so adapt to this change.
(In reply to comment #18) > Created an attachment (id=169366) [details] [review] > Remove the call to grl_log_init() in JS and vala tests > > Now that Grilo default to only print warning and error messages there's no > need to call grl_log_init() in the JS and vala tests as they won't be as > verbose as it used to be. > > Printing warning and error messages is really useful to find so > programming mistakes and thus those messages should not be discarded in > the tests. That is precisely the default behaviour of the log system, so > there's no need to call grl_log_init(). You won me on this. I had squashed those changes in your "log: remove grl_log_init() calls from tools" in my local branch. Besides, a Grl.init(0, null) is needed in the JS test app.
(In reply to comment #21) I had squashed those changes in your "log: remove > grl_log_init() calls from tools" in my local branch. Besides, a Grl.init(0, > null) is needed in the JS test app. Sounds good. I guess we should get all these patches merged and not carry on too long to throw patches in this bug. The last thing I've mentioned before is removing all the debug messages that are just "trace" messages (only printing the name of the functions) by either: - just removing them - if they are actually needed by someone, that's totally fine to leave them I guess, but they should not clutter the debug messages. we can create a new log level called "TRACE" (= DEBUG + 1 so TRACE is actually more verbose then DEBUG) and having a new macro called GRL_TRACE that uses G_STRFUNC instead of rewriting the function name every time. What do you think?
(In reply to comment #22) > (In reply to comment #21) > I had squashed those changes in your "log: remove > > grl_log_init() calls from tools" in my local branch. Besides, a Grl.init(0, > > null) is needed in the JS test app. > > Sounds good. I guess we should get all these patches merged and not carry on > too long to throw patches in this bug. The last thing I've mentioned before is > removing all the debug messages that are just "trace" messages (only printing > the name of the functions) by either: > - just removing them > - if they are actually needed by someone, that's totally fine to leave them I > guess, but they should not clutter the debug messages. we can create a new log > level called "TRACE" (= DEBUG + 1 so TRACE is actually more verbose then DEBUG) > and having a new macro called GRL_TRACE that uses G_STRFUNC instead of > rewriting the function name every time. > > What do you think? Let's first close this bug. I'm going to submits the patches to the mailing list for review and tomorrow push them if there are not further questions.
(In reply to comment #21) > (In reply to comment #18) > > Created an attachment (id=169366) [details] [review] [details] [review] > > Remove the call to grl_log_init() in JS and vala tests > > > > Now that Grilo default to only print warning and error messages there's no > > need to call grl_log_init() in the JS and vala tests as they won't be as > > verbose as it used to be. > > > > Printing warning and error messages is really useful to find so > > programming mistakes and thus those messages should not be discarded in > > the tests. That is precisely the default behaviour of the log system, so > > there's no need to call grl_log_init(). > > You won me on this. I had squashed those changes in your "log: remove > grl_log_init() calls from tools" in my local branch. Besides, a Grl.init(0, > null) is needed in the JS test app. Latest JS test app is at [1], being the needed patch in this attachment [2]. As it doesn't work due to the unsupported GParamSpec, I didn't submitted the patch upstream: just left it in the bugzilla, together with another patch which replaces GParamSpecs annotations with uints [3] (which works, but it's kind of a hack) [1] https://bugzilla.gnome.org/show_bug.cgi?id=616961 [2] https://bugzilla.gnome.org/attachment.cgi?id=168226 [3] https://bugzilla.gnome.org/attachment.cgi?id=168225
commit 3b504c0c55e0a3dfd2b3bd6b086fd9be3b0dcf75 Author: Damien Lespiau <damien.lespiau@intel.com> Date: Tue Aug 24 16:48:59 2010 +0100 log: remove grl_log_init() calls from tools grl-inspect: Now that we default to only print warnings, it's actually pretty sane to still show those warning that shows errors one should fix. grl-test-ui: The default verbosity of the test UI make seeing real warnings really hard. It's still possible to recover the verbose mode with GRL_LOG. testGrilo.js: Remove the Grl.log_init() call and add Grl.init() grilo-test.vala: Remove Grl.log_init() https://bugzilla.gnome.org/show_bug.cgi?id=627864 Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com> commit 15a83305ea12c66173db1335f3fcf74b61df25d7 Author: Damien Lespiau <damien.lespiau@intel.com> Date: Thu Sep 2 14:03:25 2010 +0100 log: rename grl_log_init() to grl_log_configure() You can configure one or several log domains at run time with this API, so it's not an initialization per se, it's closer to a configuration string one gives to the log system. Along with the renaming, properly document the format of the string used to configure log domains. https://bugzilla.gnome.org/show_bug.cgi?id=627864 commit 08805ebfa6e297e7ee369639b6af012d6ebab95d Author: Damien Lespiau <damien.lespiau@intel.com> Date: Thu Sep 2 10:49:14 2010 +0100 log: rename the environment variable to GRL_DEBUG The log system uses an environment variable to override the verbosity of log domains. Let's use GRL_DEBUG to be closer to what other gnome libraries use (GST_DEBUG, GTK_DEBUG, CLUTTER_DEBUG). https://bugzilla.gnome.org/show_bug.cgi?id=627864 commit 6c71c336c4299e0dd79ad4258ddd2e3d7cca86c3 Author: Víctor Manuel Jáquez Leal <vjaquez@igalia.com> Date: Wed Sep 1 20:12:49 2010 +0200 log: handle numeric level assignations Besides the symbolic level recognition, this patch also enable the numeric level setting. Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com> https://bugzilla.gnome.org/show_bug.cgi?id=627864 commit 9e958d6c32a6d61bc2947066d6ed4a0c9546a4f1 Author: Damien Lespiau <damien.lespiau@intel.com> Date: Fri Aug 20 17:54:52 2010 +0100 log: revamp the log system Current log system suffers from a few drawbacks: * It uses Glib's default handler for all the domains, this handler should not be overriden by a library (imagine what can happen if every library depends on being able to override the default log handler), * You can't use the common practice to break on g_log() to trace where warnings come from because the decision of printing the message is taken in the handler itself, * Other gnome libraries tend to define a single glib log domain for the whole library. So, instead, use a GStreamer-like logging system, with "log domains" you have to declare and initialize. This patch tries to keep the original author's intent by keeping: * the glib's debug levels and using them in the final g_logv(), * the same decoding of the string given to grl_log_init(). It also modifies a bit when the GRL_LOG env variable is sampled. Instead of requiring the use to explictely call grl_log_init() to check the GRL_LOG behing the scene, grilo now gets this variable at startup and overrides the default verbosity level of log domains when they are created. grl_log_init() can still be used to set the verbosity, it has to be called after the log domain has been initialized which, for plugins, means after having loaded the plugin. https://bugzilla.gnome.org/show_bug.cgi?id=627864 commit 1632777aab8cd562acf46ffeab29baeca4cb4b0b Author: Damien Lespiau <damien.lespiau@intel.com> Date: Thu Sep 2 14:22:38 2010 +0100 tests: adapt to the grl_log_init() renaming in core grl_log_init() has been renamed to grl_log_configure() in master, so adapt to this change. https://bugzilla.gnome.org/show_bug.cgi?id=627864 commit fb7f56d4a18b8cce77cfefc30100aa4fc3fbb709 Author: Damien Lespiau <damien.lespiau@intel.com> Date: Tue Aug 24 17:34:16 2010 +0100 log: use the new logging API from core This commit adapts the logging code to the new GRL_* defines and log domain code. See the corresponding commit in core for further details. https://bugzilla.gnome.org/show_bug.cgi?id=627864 Signed-off-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>