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

console: implement timeLog method #21312

Closed
wants to merge 1 commit into from
Closed

Conversation

targos
Copy link
Member

@targos targos commented Jun 13, 2018

Refs: whatwg/console#138

Marking blocked until the spec PR lands.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@targos targos added wip Issues and PRs that are still a work in progress. semver-minor PRs that contain new features and should be released in the next minor version. console Issues and PRs related to the console subsystem. labels Jun 13, 2018
@nodejs-github-bot nodejs-github-bot added the console Issues and PRs related to the console subsystem. label Jun 13, 2018
@targos targos added blocked PRs that are blocked by other issues or PRs. and removed wip Issues and PRs that are still a work in progress. labels Jun 13, 2018
lib/console.js Outdated
@@ -234,6 +234,19 @@ Console.prototype.time = function time(label = 'default') {
this._times.set(label, process.hrtime());
};

Console.prototype.timeLog = function timeLog(label = 'default', ...data) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the function name printed in the process warning, couldn't Console.prototype.timeEnd() be updated to call this?

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Barring spec changes, this looks great.

* `label` {string} **Default:** `'default'`
* `...data` {any}

For a timer that was previously started by calling [`console.time()`)][], prints
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra parenthesis in [`console.time()`)][].


```js
console.time('process');
const value = expensiveProcess1();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment that value is 42 here? Otherwise, it is a bit unexpected below.

console.time('process');
const value = expensiveProcess1();
console.timeLog('process', value);
// Prints process: 365.227ms 42
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a bit confusing. Maybe something like // Prints "process: 365.227ms 42".?

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jun 13, 2018

Should we document the separator that joins several `...data` values?

@@ -392,6 +392,25 @@ are identified by a unique `label`. Use the same `label` when calling
[`console.timeEnd()`][] to stop the timer and output the elapsed time in
milliseconds to `stdout`. Timer durations are accurate to the sub-millisecond.

### console.timeLog([label][, ...data])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this should go after ### console.timeEnd(label) section ABC-wise.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong opinion but this document doesn't seem to follow the ABC rule and I went for the spec order. It's also the order in which you call the methods

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All console. methods seem to be in ABC order according to the TOC.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. I didn't see that inspector methods were in another section

@targos
Copy link
Member Author

targos commented Jun 16, 2018

@cjihrig I refactored the common code in a separate function. PTAL.

@vsemozhetbyt

Should we document the separator that joins several ...data values?

I don't know. It's just using console.log()'s mechanics. Perhaps we should document that? I don't know how to write it though.

@vsemozhetbyt
Copy link
Contributor

@targos Then we can land as is and if anybody is sure how to document this (if needed), this can be done in another PR)

console.timeLog('process', value);
// Prints "process: 365.227ms 42".
doExpensiveProcess2(value);
console.timeEnd('process');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will print a warning about no process label, right?

Copy link
Member Author

@targos targos Jun 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not supposed to. timeLog shouldn't clear the timer.

lib/console.js Outdated
this.log('%s: %sms', label, ms.toFixed(3));
this._times.delete(label);
};
this.log('%s: %sms', label, ms.toFixed(3), ...data);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I personally would prefer to just check if data is undefined by adding a if / else but as the function is not hot, that is just a non-blocking recommendation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that 👍

@apapirovski
Copy link
Member

@jasnell
Copy link
Member

jasnell commented Jun 29, 2018

Looks like this failed across the board.

@targos
Copy link
Member Author

targos commented Jun 30, 2018

@targos targos removed the blocked PRs that are blocked by other issues or PRs. label Jul 4, 2018
@targos
Copy link
Member Author

targos commented Jul 4, 2018

Upstream PR landed: https://console.spec.whatwg.org/#timelog

@targos
Copy link
Member Author

targos commented Jul 4, 2018

@targos
Copy link
Member Author

targos commented Jul 4, 2018

Landed in b016745

@targos targos closed this Jul 4, 2018
@targos targos deleted the console-timelog branch July 4, 2018 20:14
targos added a commit that referenced this pull request Jul 4, 2018
Refs: whatwg/console#138

PR-URL: #21312
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this pull request Jul 6, 2018
Refs: whatwg/console#138

PR-URL: #21312
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this pull request Jul 17, 2018
Notable changes:

* console:
  * The `console.timeLog()` method has been implemented. (#21312)
* deps:
  * Upgrade to libuv 1.22.0. (#21731)
  * Upgrade to ICU 62.1 (Unicode 11, CLDR 33.1). (#21728)
* http:
  * Added support for passing both `timeout` and `agent` options to
    `http.request`. (#21204)
* napi:
  * Added experimental support for functions dealing with bigint numbers. (#21226)
* process:
  * The `process.hrtime.bigint()` method has been implemented. (#21256)
  * Added the `--title` command line argument to set the process title on
    startup. (#21477)
* trace_events:
  * Added process_name metadata. (#21477)
@targos targos mentioned this pull request Jul 17, 2018
targos added a commit that referenced this pull request Jul 18, 2018
Notable changes:

* console:
  * The `console.timeLog()` method has been implemented. (#21312)
* deps:
  * Upgrade to libuv 1.22.0. (#21731)
  * Upgrade to ICU 62.1 (Unicode 11, CLDR 33.1). (#21728)
* http:
  * Added support for passing both `timeout` and `agent` options to
    `http.request`. (#21204)
* napi:
  * Added experimental support for functions dealing with bigint numbers. (#21226)
* process:
  * The `process.hrtime.bigint()` method has been implemented. (#21256)
  * Added the `--title` command line argument to set the process title on
    startup. (#21477)
* trace_events:
  * Added process_name metadata. (#21477)
* Added new collaborators
  * codebytere - Shelley Vohr

PR-URL: #21851
targos added a commit that referenced this pull request Jul 18, 2018
Notable changes:

* console:
  * The `console.timeLog()` method has been implemented.
    (#21312)
* deps:
  * Upgrade to libuv 1.22.0. (#21731)
  * Upgrade to ICU 62.1 (Unicode 11, CLDR 33.1).
    (#21728)
* http:
  * Added support for passing both `timeout` and `agent` options to
    `http.request`. (#21204)
* inspector:
  * Expose the original console API in `require('inspector').console`.
    (#21659)
* napi:
  * Added experimental support for functions dealing with bigint numbers.
    (#21226)
* process:
  * The `process.hrtime.bigint()` method has been implemented.
    (#21256)
  * Added the `--title` command line argument to set the process title on
    startup. (#21477)
* trace_events:
  * Added process_name metadata.
    (#21477)
* Added new collaborators
  * codebytere - Shelley Vohr

PR-URL: #21851
targos added a commit that referenced this pull request Jul 18, 2018
Notable changes:

* console:
  * The `console.timeLog()` method has been implemented.
    (#21312)
* deps:
  * Upgrade to libuv 1.22.0. (#21731)
  * Upgrade to ICU 62.1 (Unicode 11, CLDR 33.1).
    (#21728)
* http:
  * Added support for passing both `timeout` and `agent` options to
    `http.request`. (#21204)
* inspector:
  * Expose the original console API in `require('inspector').console`.
    (#21659)
* napi:
  * Added experimental support for functions dealing with bigint numbers.
    (#21226)
* process:
  * The `process.hrtime.bigint()` method has been implemented.
    (#21256)
  * Added the `--title` command line argument to set the process title on
    startup. (#21477)
* trace_events:
  * Added process_name metadata.
    (#21477)
* Added new collaborators
  * codebytere - Shelley Vohr

PR-URL: #21851
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console Issues and PRs related to the console subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants