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

makes formatting of log output to be configurable and plugable #9799

Merged
merged 5 commits into from Oct 14, 2019

Conversation

@johnspackman
Copy link
Member

commented Oct 10, 2019

The initial motivation of this PR was to allow the datetime formatting to be changed in log output - by default, the logging outputs the time as the number of milliseconds since the app started, which is concise and great for browser apps, but for long running node apps it's very inconvenient because what you really need to know is an actual date/time so that log entries can be related to real human events (like an end-user's bug report). Changing the logging output (without this PR) is not possible because all appenders are baked in to use the static method qx.log.appender.Util.toText()

This PR changes the logging appenders so that they use an instance of the new qx.log.appender.Formatter; there is a default instance of Formatter (which can be replaced) and it has helper methods that can be used to put together the full logging output.

Also in this PR is a change to appenders - previously, every appender is uniquely identified by it's class name, but this prevents using the same class more than once; EG for node apps, it's quite reasonable to want to have a file-based appender, and to have multiple instances of a FileAppender, each one with a different set of filters and output filename.

This PR allows the appender to define a static appenderName property that will be used in preference to the classname property as a unique identifier. This is obviously pretty ugly, but appenders are static classes at the moment and all appenders work by "convention" of having well known exposed static methods; this PR is deliberately not attempting to rewrite the entire Appender class implementation.

Copy link
Member

left a comment

Sorry, still early morning, but how does it help to set an appenderName if the appender is static? There will always only be one ...

@johnspackman

This comment has been minimized.

Copy link
Member Author

commented Oct 11, 2019

Because it's only the current appenders which are static - qx.log.Logger relies on being given a "thing" which supports a method called process() and a property called classname (and now also appenderName). That "thing" could be a static class or an instance of a class derived from qx.core.Object, or probably just { process() {}, classname: "abc" }.

This appenderName is useful if you have a proper object instance with a class derived from qx.core.Object - that instance will already have a classname via the prototype plus a non-static, overridable method called process. You can pass this [non-static implementation of an] object as an appender, but the appenders need a unique ID.

IMHO the appenders need to be rewritten as a proper class, rather than this non-obvious kludge, but I want to keep that out of scope for now because there's quite a lot of code in the current appenders - plus it looks like there were some dependency issues that the original author was worried about.

@oetiker

This comment has been minimized.

Copy link
Member

commented Oct 11, 2019

ah ... it does not HAVE to be static ... in that case :) perfect

@@ -172,7 +172,8 @@ qx.Class.define("qx.log.appender.Console",
process : function(entry)
{
// Append new content
this.__log.appendChild(qx.log.appender.Util.toHtml(entry));
var formatter = qx.log.appender.Formatter.getDefaultFormatter();

This comment has been minimized.

Copy link
@derrell

derrell Oct 11, 2019

Member

If the goal here is to be able to use different formatters, then why would we always retrieve the default formatter? Should this instead be

var formatter = qx.log.appender.Formatter.getFormatter();

and the formatter property just has a default value, but it can be changed?

*
* @param instance {qx.log.appender.Formatter}
*/
setDefaultFormatter: function(instance) {

This comment has been minimized.

Copy link
@derrell

derrell Oct 11, 2019

Member

Yeah, this seems hokey to me. The default shouldn't be changeable; rather, there should be a default, I think, that getFormatter returns until setFormatter changes what is to be used.

This comment has been minimized.

Copy link
@johnspackman

johnspackman Oct 14, 2019

Author Member

done

This comment has been minimized.

Copy link
@hkollmann

hkollmann Oct 14, 2019

Member

No the test must be changed too.,.,.

level420 and others added 3 commits Oct 14, 2019
…nto logging-formatter

* 'logging-formatter' of github.com:johnspackman/qooxdoo:
  fixes a bug with mixins when added via `qx.Class.patch` (#9800)
@hkollmann hkollmann merged commit cfe2f9f into qooxdoo:master Oct 14, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pullapprove 1 group approved
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.