-
Notifications
You must be signed in to change notification settings - Fork 80
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
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?
@@ -279,21 +276,16 @@ export const log = (...args: any): void => { | |||
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 sorting
https://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
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 Ids
to be tracked ifdebug
is not enabled onLitNodeClient
by allowing logging calls to pass through theLogger
instance.Adds time based sorting to the array of
requests Ids
returned bygetRequestIds
onLitCore