Missing stack traces and some questions

[arggh]

Missing stack trace

If I catch an error on the client and attempt to log it like so:

logger.error('Something wonky happened', { error: e });

Then, the entry in DB is missing the stack trace. The error object only consists of properties line, column, sourceURL.

If I include the stack trace in the context manually like so:

logger.error('Something wonky happened', {
   stack: e.stack,
   error: e
});

-> Stack trace is present in the DB entry.

Is this intentional, if so, what is the reasoning behind it?

Logging the error objects

What about logging errors like so:

try {
   something();
} catch (e) {
   // something something if condition
   ...
   // I've no clue what happened, better just log the whole error
   logger.error(e);
}

This produces an error log entry that's not helpful at all:

{
    "level": 3,
    "message": "[object Object]",
    "context": {
        // ...whatever processors you had

Is there a reason FiLog is not producing sensible output from this type of use?

Completely swallowing the error event

Should I try to log like this:

logger.error(error.message, error);

I get nothing logged, console or DB. It just silently does nothing. This is a bit dangerous, no?

[arggh]

Stack traces are normally collected if you arm() the logger only: in that case you don't have to provide them.

I do call logger.arm(), but the scenario I'm testing here, is when I have a try-catch, in which I log the error in the catch block:

try {
   thisWillDefinitelyThrowError();
} catch (error) {
   ...
   logger.error(error.message, { error });
}

That will not include the stack trace of the error object. I have to include it manually, like so:

logger.error(error.message, { error: e, stack: e.stack });

The other points were more like ideas about some shorthands, if you will. But maybe they are unnecessary. However, the last point I think at least deserves a warning for the developer:

If the expected arguments are message:String and context:Object, I would not probably be the first to try this: logger.error(error.message, error);, which in fact doesn't do anything.

[fgm]

Here is how I do it when catching manually (not using arm()):

try {
  throw new Error("oops in app client");
}
catch (e) {
  const normalized = window.logger.tk.computeStackTrace(e);
  window.logger.error(normalized.message, normalized);
}

From my checks, it gives correct logging with a stack, like this:

{
        "_id" : "pxCJDkmX4GrWe8z6M",
        "level" : 3,
        "message" : "oops in app client",
        "context" : {
                "mode" : "stack",
                "name" : "Error",
                "message" : "oops in app client",
                "stack" : [
                        {
                                "url" : "http://localhost:3000/app/app.js?hash=376940e55bc0168a0cd79d49cebe76983a12eb7b",
                                "func" : "meteorInstall.client.main.js",
                                "args" : [ ],
                                "line" : 103,
                                "column" : 9,
                                "context" : [
                                        "                                                                                                           //",
                                        "Meteor.startup(function () {});                                                                            // 48",
[..]

Does this fail for you ?

[arggh]

That works, however, wouldn't it be nice, if these two were equal?

const normalized = window.logger.tk.computeStackTrace(e);
window.logger.error(normalized.message, normalized);

window.logger.error(e);

[fgm]

Not sure what a consistent easy to use interface/signature could be. Do you have a suggestion of how it could be improved with a documentable spec (à la Jsdoc) ?

At any rate, I think this question, and others before it, confirm doc needs to be rethought. Currently, it mostly enumerates the components, but I could probably be more useful as a set of examples, and moving the documented enumeration to the actual doc pages instead of the README, don't you think ?

[arggh]

I think the doc could be improved with better structure by organizing content and providing more examples, instead of just being a long list of "random snippets of information" about the package.

Also, regarding the last message about comparing the two ways of logging a manually caught errors, I think it would be nice not to force the developer to jump through hoops by calling window.logger.tk.computeStackTraceand normalized.message to just log an error.

moving the documented enumeration to the actual doc pages instead of the README, don't you think

I think this is an excellent idea. Having a brief introduction and more tutorial-like-content in the README is preferable, in my opinion.

[arggh]

As a sidenote, the README on npmjs appears blank: https://www.npmjs.com/package/filog

[fgm]

About the README, yes I noticed that: I added "readme" clause in the package.json, as suggested by validation, but it is not described in the package.json spec and apparently contains the actual readme instead of the file name of the readme as expected: it will be reverted in the next release, but I don't think it warrants doing yet another release just for that.

Agreed it would be nice to make things simpler, as you say, but what I have no idea about at this point is how to make it as simple as possible (but not simpler), so if you have a clear idea (hence the notion of how it could look as a jsdoc comment), I'm really interested.

[arggh]

Well, if I understood correctly, here goes:

Actually the second scenario in the first post ("Logging the error objects") is perfectly valid according to the current documentation for log method (wrapped in error, debug, info etc), except for the rawContext and cooked parameters which are not documented as optional, even though they have default values declared.

It says the message can be a String or an Object with message-key, right? Yet the message that gets logged that way is [object Object].

Current JSDoc is here:

/**
   * Log an event. This is the *MAIN* method in the whole package.
   *
   * @param {Number} level
   *   An RFC5424 severity level.
   * @param {Object|String} message
   *   - If it is a string, the message body
   *   - Otherwise it must be an object with a "message" key.
   *   It may contain placeholders to be substituted with values from the
   *   context object, as in PSR-3.
   * @param {Object} rawContext
   *   An object complementing the message.
   * @param {Boolean} cooked
   *   Is the context already reduced ?
   *
   * @returns {void}
   */
  log(level, message, rawContext = {}, cooked = true) {
      ...

I would change it like this:

/**
   * Log an event. This is the *MAIN* method in the whole package.
   *
   * @param {Number} level
   *   An RFC5424 severity level.
   * @param {Object|String} message
   *   - If it is a string, the message body
   *   - Otherwise it must be an object with a "message" key.
   *   - If "rawContext" is not provided, the object will be used as context for the log event
   * @param {Object} [rawContext]
   *   An object complementing the message.
   * @param {Boolean} [cooked]
   *   Is the context already reduced ?
   *
   * @returns {void}
   */
  log(level, message, rawContext = {}, cooked = true) {
      ...

or maybe state in the rawContext param:

 * @param {Object} [rawContext]
 *   An object complementing the message. If no parameter is provided, the first argument will be used as context

So in essence after the method logic complies to the specification, these two would have equal result:

...
} catch (e) {
   logger.error(e);
}

...
} catch (e) {
   const normalized = logger.tk.computeStackTrace(e);
   logger.error(normalized.message, normalized);
}