-
Notifications
You must be signed in to change notification settings - Fork 88
Feature/lit 3911 add request id aggregation to logger when logging is disabled #661
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
Feature/lit 3911 add request id aggregation to logger when logging is disabled #661
Conversation
| OFF = -1, | ||
| ERROR = 0, | ||
| INFO = 1, | ||
| DEBUG = 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: These orderings are not standard. going to come back and re work the level enumeration weights in a fast follow PR with other changes we discussed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use the levels from another lib like ethers?
…n-to-logger-when-logging-is-diabled
MaximusHaximus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm -- The logXXXX methods from misc seem to be imported into a few more places that aren't passing a 'logger' as the first argument to them -- at first glance, I see contracts-sdk, lit-node-client-nodejs, crypto, encryption and EthWalletProvider, also logError in pkp-base. Do we need to update all of those places too? Do we really need to add logger as the first arg to these functions to get the request IDs to print or is that an opportunistic change? 🤔
Added a couple of questions in-line, but also, my apologies if I'm missing something obvious, but I'm having some difficulty understanding how this doesn't result in logging being fully enabled even when debug is disabled entirely -- what keeps us from logging all logs now that we don't exit early in misc methods?
| return; | ||
| } | ||
|
|
||
| if (globalThis?.litConfig?.debug !== true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Why does removing this check and the one on L310-L312 below not result in logging just being blanket enabled even when debug is set to false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look in core where the logger is created we now will set the logger to OFF which will prevent logs from outputting as it won't have a handler, nor will logs be stored. But it will allow a new logger child to be created which will aggregate request ids.
b66b0fd to
f40c203
Compare
|
|
||
| return keys; | ||
| return keys | ||
| .sort((a: [string, number], b: [string, number]) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the goal here is to sort the array keys based on the timestamp (child[1].timestamp]) right? If so, I think we need to do the following instead, because the sorting callback should return a positive number, a negative number, and a 0 if they are equal.
return keys
.sort((a: [string, number], b: [string, number]) => {
return a[1] - b[1];
}) // Corrected sortinghttps://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort
A comparator conforming to the constraints above will always be able to return all of 1, 0, and -1, or consistently return 0. For example, if a comparator only returns 1 and 0, or only returns 0 and -1, it will not be able to sort reliably because anti-symmetry is broken. A comparator that always returns 0 will cause the array to not be changed at all, but is reliable nonetheless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! good catch, I had this refactored to the same but I did not push it. Since any value < 0 will result in: [a,b] or the original as the sort result this is rather elegant.
…n-to-logger-when-logging-is-diabled
Ansonhkg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Allows for
request Idsto be tracked ifdebugis not enabled onLitNodeClientby allowing logging calls to pass through theLoggerinstance.Adds time based sorting to the array of
requests Idsreturned bygetRequestIdsonLitCore