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

util: allow wildcards in debuglog() #17609

Closed
wants to merge 3 commits into from

Conversation

TylerYang
Copy link
Contributor

@TylerYang TylerYang commented Dec 11, 2017

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

Affected core subsystem(s)
util

fixes #17605

@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Dec 11, 2017
@TylerYang TylerYang mentioned this pull request Dec 11, 2017
4 tasks
@TylerYang TylerYang changed the title Tyler refactor debuglog util: allow wildcards in debuglog() Dec 11, 2017
@Fishrock123
Copy link
Contributor

I think this is a great idea. Do you think you'd be able to rebase out the merge commit?

@TylerYang
Copy link
Contributor Author

@Fishrock123 Yes, just rebased. Feel free to add more comments : )

lib/util.js Outdated
.replace(/\*/g, '.*');
});

return new RegExp(strArr.join('|'), 'i');
Copy link
Member

@targos targos Dec 12, 2017

Choose a reason for hiding this comment

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

The RegExp must enforce start and end: new RegExp('^(' + strArr.join('|') + ')$'), otherwise any string acts as one with wildcards at the beginning and end (and since the default is an empty string, not setting NODE_DEBUG results in all debug messages being printed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx, updated.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

A few small comments that should be addressed, but otherwise LGTM.

lib/util.js Outdated
.replace(/\*/g, '.*');
});

return new RegExp(`^${strArr.join('|')}$`, 'i');
Copy link
Member

Choose a reason for hiding this comment

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

Instead of calling map and join you could use reduce

const regExpStr = strArr.reduce((str, entry, i) => {
  let part = entry.replace(regExpSpecialChars, '\\$&')
    .replace(/\*/g, '.*');
  if (i -1 !== strArr.length)
    part += '|';
  return `${str}${part}`;
}, '');
return new RegExp(`^${regExpStr}$`, 'i');

But that might just be my personal preference.

Copy link
Member

Choose a reason for hiding this comment

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

I think the current code is a bit more readable (not having to deal with the indices is nice, at least :))

lib/util.js Outdated
debugEnviron = new Set(
(process.env.NODE_DEBUG || '').split(',').map((s) => s.toUpperCase()));
debugEnviron = Array.from(new Set(
(process.env.NODE_DEBUG || '').split(',').map((s) => s.toUpperCase())));
Copy link
Member

Choose a reason for hiding this comment

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

This part is weird. First an Array is created, then the Set, then an Array from the Set.

Copy link
Member

Choose a reason for hiding this comment

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

@BridgeAR This happens for deduplication, do you have other suggestions? Converting to a Set and back is usualy what I do too

Copy link
Contributor Author

@TylerYang TylerYang Dec 13, 2017

Choose a reason for hiding this comment

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

The only thing I can think of is improving readability by adding one more statement like,

debugEnviron = (process.env.NODE_DEBUG || '').split(',').map((s) => s.toUpperCase())
debugEnviron = Array.from(new Set(debugEnviron));

But i wonder if it worth.

Copy link
Contributor Author

@TylerYang TylerYang Dec 13, 2017

Choose a reason for hiding this comment

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

wait, we can convert NODE_DEBUG to uppercase before splitting so that we could remove the map operator.

debugEnviron = (process.env.NODE_DEBUG || '').toUpperCase().split(',');
debugEnviron = Array.from(new Set(debugEnviron));

or just

debugEnviron = Array.from(new Set((process.env.NODE_DEBUG || '').toUpperCase().split(',')));

what do you think? @BridgeAR @addaleax

Copy link
Member

Choose a reason for hiding this comment

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

I think it's just that we use [...spread] syntax more than Array.from(), which has the added advantage of being safer in case Array or Array.from is overridden. In any case I'd prefer the first variant for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx @TimothyGu Nice solution! Updated.

Copy link
Member

@BridgeAR BridgeAR Dec 13, 2017

Choose a reason for hiding this comment

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

@addaleax the current implementation was not about deduplication, see #13841. I guess it was chosen to make sure you do not have to lookup the entry in a array as that is slower. Depulication for a environment variable that is passed in by the user is definitely surprising to me. I would expect people not to include the same entry multiple times ;-)
As this is a one time thing and debugging is always slower, I will not block it from being included if you really think that is necessary (even though I do not see the point in it. Nothing bad can happen if a entry is a duplicate).

What I would like is to move the replacement to before the splitting. Running the RegExp once is a lot better than running it for each entry (even though there are probably very few).

I would also argue that the part being lazy does not make all that much sense here and I would rather move it out instead (it became lazy in a independent commit 22c68fd).

Copy link
Member

Choose a reason for hiding this comment

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

In case the replacement is done right away, the splitting and joining could also be replaced by a replace().

Copy link
Contributor Author

@TylerYang TylerYang Dec 14, 2017

Choose a reason for hiding this comment

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

Hi @BridgeAR . Thx for details, appreciate that!

What I would like is to move the replacement to before the splitting. Running the RegExp once is a lot better than running it for each entry (even though there are probably very few).

make sense, thx

In case the replacement is done right away, the splitting and joining could also be replaced by a replace().

Yes, it could. Would this reduce the readability?
Just try, it's more intuitive. thx.

doc/api/util.md Outdated
it will output something like:

```txt
FOO 3245: hello from foo [123]
FOO-BAR 3245: hello from foo [123]
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to have another example specifically for the wildcard. Including counter examples. Especially people who are new to coding might want to have more explicit explanations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx, added.

doc/api/util.md Outdated
@@ -75,7 +75,7 @@ is wrapped in an `Error` with the original value stored in a field named
added: v0.11.3
-->

* `section` {string} A string identifying the portion of the application for
* `section` {string} A wildcard string identifying the portion of the application for
Copy link
Member

Choose a reason for hiding this comment

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

The string itself is not a wildcard string as long as there is no asterisk included.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx, updated.

@TylerYang TylerYang force-pushed the tyler-refactor-debuglog branch 2 times, most recently from 560a840 to 7a998e1 Compare December 13, 2017 08:33
@TylerYang TylerYang force-pushed the tyler-refactor-debuglog branch 2 times, most recently from b7b0c51 to 9d8ce29 Compare December 14, 2017 03:52
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Almost LGTM

lib/util.js Outdated
}
set = set.toUpperCase();
if (!debugs[set]) {
if (debugEnviron.has(set)) {
const regex = stringToRegExp(debugEnv);
Copy link
Member

@BridgeAR BridgeAR Dec 14, 2017

Choose a reason for hiding this comment

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

The RegExp should only be calculated once. It has to be done in the if (debugEnv === undefined) { ... } check. I personally would actually move that out of the debuglog to be not lazy (meaning the NODE_DEBUG env should be checked when util is required and the RegExp should also be created at that point).

E.g.

let debugRegExp
if (process.env.NODE_DEBUG) {
  debugEnv = process.env.NODE_DEBUG.replace(...).replace(...).replace(...).toUpperCase()
  debugRegExp = new RegExp(...)
}

function debuglog(set) { ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, updated.

@apapirovski
Copy link
Member

@TylerYang It doesn't appear that any of the commits here associated with your GitHub account. See the Contributing guide for info on how to fix that, as otherwise you won't get the GitHub "Contributor" badge next to your name.

@TylerYang
Copy link
Contributor Author

@apapirovski Thx for pointed out! Updated my account to the github one.

lib/util.js Outdated

let debugEnvRegex;
const regExpSpecialChars = /[|\\{}()[\]^$+?.]/g;
if (debugEnvRegex === undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

This if statement will always evaluates to true. It should actually test for process.env.NODE_DEBUG and only if that is truthy it should be evaluated.

This is how I would write it

const debugs = {};
let debugEnvRegex = /^$/;
if (process.env.NODE_DEBUG) {
  const debugEnv = process.env.NODE_DEBUG
    // Escape RegExp special characters
    .replace(/[|\\{}()[\]^$+?.]/g, '\\$&')
    .replace(/\*/g, '.*')
    .replace(/,/g, '$|^')
    .toUpperCase();
  debugEnvRegex = new RegExp(`^${debugEnv}$`, 'i');
}

Copy link
Contributor Author

@TylerYang TylerYang Dec 14, 2017

Choose a reason for hiding this comment

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

You are right! I should consider that corner case at the first time. Updated. Thx : )

@BridgeAR
Copy link
Member

@TylerYang one minor issue left. Thanks a lot for being so patient!

lib/util.js Outdated
const debugs = {};

let debugEnvRegex = /^$/;
const regExpSpecialChars = /[|\\{}()[\]^$+?.]/g;
Copy link
Member

Choose a reason for hiding this comment

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

Nit - please move this into the if. There is no point in keeping the variable around after the RegExp got created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@targos
Copy link
Member

targos commented Dec 15, 2017

@targos targos added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 15, 2017
@targos
Copy link
Member

targos commented Dec 15, 2017

Whoever lands this, feel free to squash all commits and remove my authorship

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Dec 15, 2017
PR-URL: nodejs#17609
Fixes: nodejs#17605
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@BridgeAR
Copy link
Member

Landed in fafd9fb

@TylerYang congratulations on your first commit to Node.js! 🎉

@BridgeAR BridgeAR closed this Dec 15, 2017
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
PR-URL: #17609
Fixes: #17605
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
MylesBorins added a commit that referenced this pull request Jan 10, 2018
Notable change:

* async_hooks:
  - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither
    API were documented. (Andreas Madsen)
    #16972
* deps:
  - update nghttp2 to 1.29.0 (James M Snell)
    #17908
  - upgrade npm to 5.6.0 (Kat Marchán)
    #17535
  - cherry-pick 50f7455 from upstream V8 (Michaël Zasso)
    #16591
* events:
  - remove reaches into _events internals (Anatoli Papirovski)
    #17440
* http:
  - add rawPacket in err of `clientError` event (XadillaX)
    #17672
* http2:
  - implement maxSessionMemory (James M Snell)
    #17967
  - add initial support for originSet (James M Snell)
    #17935
  - add altsvc support (James M Snell)
    #17917
  - perf_hooks integration (James M Snell)
    #17906
* net:
  - remove Socket.prototype.write (Anna Henningsen)
    #17644
  - remove Socket.prototype.listen (Ruben Bridgewater)
    #13735
* repl:
  - show lexically scoped vars in tab completion (Michaël Zasso)
    #16591
* stream:
  - rm {writeable/readable}State.length (Calvin Metcalf)
    #12857
  - add flow and buffer properties to streams (Calvin Metcalf)
    #12855
* util:
  - allow wildcards in NODE_DEBUG variable (Tyler)
    #17609
* zlib:
  - add ArrayBuffer support (Jem Bezooyen)
    #16042
* Addedew collaborator**
  - [starkwang](https://github.com/starkwang) Weijia Wang
* Addedew TSC member**
  - [danbev](https://github.com/danbev) Daniel Bevenius

PR-URL: #18069
MylesBorins added a commit that referenced this pull request Jan 10, 2018
Notable change:

* async_hooks:
  - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither
    API were documented. (Andreas Madsen)
    #16972
* deps:
  - update nghttp2 to 1.29.0 (James M Snell)
    #17908
  - upgrade npm to 5.6.0 (Kat Marchán)
    #17535
  - cherry-pick 50f7455 from upstream V8 (Michaël Zasso)
    #16591
* events:
  - remove reaches into _events internals (Anatoli Papirovski)
    #17440
* http:
  - add rawPacket in err of `clientError` event (XadillaX)
    #17672
* http2:
  - implement maxSessionMemory (James M Snell)
    #17967
  - add initial support for originSet (James M Snell)
    #17935
  - add altsvc support (James M Snell)
    #17917
  - perf_hooks integration (James M Snell)
    #17906
  - Refactoring and cleanup of Http2Session and Http2Stream destroy
    (James M Snell) #17406
* net:
  - remove Socket.prototype.write (Anna Henningsen)
    #17644
  - remove Socket.prototype.listen (Ruben Bridgewater)
    #13735
* repl:
  - show lexically scoped vars in tab completion (Michaël Zasso)
    #16591
* stream:
  - rm {writeable/readable}State.length (Calvin Metcalf)
    #12857
  - add flow and buffer properties to streams (Calvin Metcalf)
    #12855
* util:
  - allow wildcards in NODE_DEBUG variable (Tyler)
    #17609
* zlib:
  - add ArrayBuffer support (Jem Bezooyen)
    #16042
* Addedew collaborator**
  - [starkwang](https://github.com/starkwang) Weijia Wang
* Addedew TSC member**
  - [danbev](https://github.com/danbev) Daniel Bevenius

PR-URL: #18069
MylesBorins added a commit that referenced this pull request Jan 10, 2018
Notable change:

* async_hooks:
  - deprecate AsyncHooks Sensitive API and runInAsyncIdScope. Neither
    API were documented. (Andreas Madsen)
    #16972
* deps:
  - update nghttp2 to 1.29.0 (James M Snell)
    #17908
  - upgrade npm to 5.6.0 (Kat Marchán)
    #17535
  - cherry-pick 50f7455 from upstream V8 (Michaël Zasso)
    #16591
* events:
  - remove reaches into _events internals (Anatoli Papirovski)
    #17440
* http:
  - add rawPacket in err of `clientError` event (XadillaX)
    #17672
* http2:
  - implement maxSessionMemory (James M Snell)
    #17967
  - add initial support for originSet (James M Snell)
    #17935
  - add altsvc support (James M Snell)
    #17917
  - perf_hooks integration (James M Snell)
    #17906
  - Refactoring and cleanup of Http2Session and Http2Stream destroy
    (James M Snell) #17406
* net:
  - remove Socket.prototype.write (Anna Henningsen)
    #17644
  - remove Socket.prototype.listen (Ruben Bridgewater)
    #13735
* repl:
  - show lexically scoped vars in tab completion (Michaël Zasso)
    #16591
* stream:
  - rm {writeable/readable}State.length (Calvin Metcalf)
    #12857
  - add flow and buffer properties to streams (Calvin Metcalf)
    #12855
* util:
  - allow wildcards in NODE_DEBUG variable (Tyler)
    #17609
* zlib:
  - add ArrayBuffer support (Jem Bezooyen)
    #16042
* Addedew collaborator**
  - [starkwang](https://github.com/starkwang) Weijia Wang
* Addedew TSC member**
  - [danbev](https://github.com/danbev) Daniel Bevenius

PR-URL: #18069
@TimothyGu TimothyGu removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 13, 2018
@MylesBorins MylesBorins added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 23, 2018
@MylesBorins
Copy link
Contributor

Seems like this should have perhaps been labelled semver-minor... is that mistaken?

@BridgeAR
Copy link
Member

@MylesBorins I totally agree.

@MylesBorins
Copy link
Contributor

This would likely be useful for 8.x, but it may rely on a Semver-Major commit. Can someone confirm this is OK to land and desirable for 8.x

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

Successfully merging this pull request may close these issues.

allow wildcard match in util.debuglog()