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

Support custom timestamp format #6

Closed
wants to merge 3 commits into from

Conversation

@fhemberger
Copy link
Contributor

fhemberger commented Nov 25, 2014

Allow an optional argument to set a custom format for the log timestamp:

Example configuration for hapi:

var options = {
    reporters: [{
        reporter: require('good-console'),
        args:[
            { log: '*', request: '*'},
            // ISO 8601 date format
            { timestampFormat: 'YYYY-MM-DDTHH:mm:ss.SSS[Z]' }
        ]
    }]
};
@@ -13,7 +13,7 @@ var internals = {};
module.exports = internals.GoodConsole = function (events, options) {

Hoek.assert(this.constructor === internals.GoodConsole, 'GoodConsole must be created with new');
options = options || {};
internals.options = options = options || {};

This comment has been minimized.

Copy link
@arb

arb Nov 25, 2014

Contributor

Since internals is shared across instances, we really don't ever want to see it on the left hand side of an equals sign unless this object is only instantiated once, and good-console can be instantiated more than once.


var m = Moment.utc(timestamp);
return m.format('YYMMDD/HHmmss.SSS');
return m.format(format || 'YYMMDD/HHmmss.SSS');

This comment has been minimized.

Copy link
@arb

arb Nov 25, 2014

Contributor

Use something like Hoek.applyDefaults instead of doing the comparison inline (and every time).

};

internals.printEvent = function (event) {

var timestring = internals.GoodConsole.timeString(event.timestamp);
var timestring = internals.GoodConsole.timeString(event.timestamp, internals.options.timestampFormat);

This comment has been minimized.

Copy link
@arb

arb Nov 25, 2014

Contributor

You should have access here to this._settings.format. Also, please rename to format so it's consistent with good-file.

This comment has been minimized.

Copy link
@fhemberger

fhemberger Nov 25, 2014

Author Contributor

Unfortunately, 'settings' doesn't seem to be exposed. That's why I added it first to 'internals'

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Nov 25, 2014

I think this should also be documented in the READMe. Not a whole write up, just a single line about the option.

@arb arb added this to the 2.1.0 milestone Nov 25, 2014
@arb arb self-assigned this Nov 25, 2014
@arb arb added the feature label Nov 25, 2014
@fhemberger

This comment has been minimized.

Copy link
Contributor Author

fhemberger commented Nov 25, 2014

Thanks for the feedback, I'll update the PR …

@fhemberger

This comment has been minimized.

Copy link
Contributor Author

fhemberger commented Nov 25, 2014

You have only access to this._settings on the prototype methods, not in GoodConsole.timeString. Therefore I need to format the timestamp directly in _report.

@@ -82,6 +84,8 @@ internals.printRequest = function (event) {

internals.GoodConsole.prototype._report = function (event, eventData) {

eventData.timestamp = internals.GoodConsole.timeString(eventData.timestamp, this._settings.format);

This comment has been minimized.

Copy link
@arb

arb Nov 25, 2014

Contributor

Can't do that. This is a reference and other reporters are using the timestamp, this will change it for any other reporters "downstream" for this event. You're going to have to create a string format similar to how this was working before in printEvent without resetting eventData.timestamp

This comment has been minimized.

Copy link
@fhemberger

fhemberger Nov 26, 2014

Author Contributor

Sorry, of cause you're right … I tried to avoid the call to Hoek. applyToDefaults in each timeString call (got a bit distracted). This will be the best option, I think:

  • Use the format parameter in timeString, apply to default value on each call
  • Get custom format coming from this._settings.format in _report, set as second parameter for printRequest and printEvent.
  • Change back the unit tests

This comment has been minimized.

Copy link
@arb

arb Nov 26, 2014

Contributor
  • Change signature of printEvent to take the event and the format
  • Keep the hoek.merge logic so that each instance has this._settings.format attached to it
  • Pass in event and this._settings.formateverywhere printEvent is called
@arb

This comment has been minimized.

Copy link
Contributor

arb commented Nov 26, 2014

You'll also need to pull from upstream as master has changed since this PR was opened.

@fhemberger

This comment has been minimized.

Copy link
Contributor Author

fhemberger commented Nov 30, 2014

Ok, I have it fixed by now, there is only one thing before submitting the updates PR:
In the tests, you call GoodConsole.timeString directly with a timestamp without a format (and I think you want to keep this behavior). However, when no format is passed, Moment.js uses it's default format, which is ISO 8601.

I'm not sure, how you want to handle this: One way would be to re-apply internals.defaults.format in GoodConsole.timeString (in case the method has been called directly), so this operation is done twice, once in the constructor and again in timeString, or change the default date format to ISO 8601 (i.e. no format passed in) and use your previous default format ('YYMMDD/HHmmss.SSS') as custom format.

module.exports = internals.GoodConsole = function (events, options) {

    Hoek.assert(this.constructor === internals.GoodConsole, 'GoodConsole must be created with new');
    options = options || {};
    var settings = Hoek.applyToDefaults(internals.defaults, options);

    GoodReporter.call(this, events, settings);
};

Hoek.inherits(internals.GoodConsole, GoodReporter);

internals.GoodConsole.timeString = function (timestamp, format) {

    format = Hoek.applyToDefaults(internals.defaults, { format: options });
    var m = Moment.utc(timestamp);
    return m.format(format);
};

I have to say that I would favor the second approach, as ISO 8601 is the standardized date format (e.g. '2014-03-30T19:28:55.000+00:00'), whereas 'YYMMDD/HHmmss.SSS' (as far as I know) is not.

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Nov 30, 2014

Change timeString, printEvent, and printRequest to all take format as a second parameter. In the tests, just pass a valid format string. timeString is really only public because of testing and shouldn't be called directly. In fact, you could prefix the method with _ while you are in there.

@fhemberger

This comment has been minimized.

Copy link
Contributor Author

fhemberger commented Dec 1, 2014

Alright, I'll change it then and submit a clean PR later, so you don't have to deal with all that single commits going back and forth. ;)

@fhemberger

This comment has been minimized.

Copy link
Contributor Author

fhemberger commented Dec 1, 2014

Closed, new PR is #12

@fhemberger fhemberger closed this Dec 1, 2014
arb added a commit that referenced this pull request Dec 3, 2014
Support custom timestamp format, see #6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.