Skip to content

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

Conversation

joshLong145
Copy link

Allows for request Ids to be tracked if debug is not enabled on LitNodeClient by allowing logging calls to pass through the Logger instance.

Adds time based sorting to the array of requests Ids returned by getRequestIds on LitCore

@joshLong145 joshLong145 changed the title Feature/lit 3911 add request id aggregation to logger when logging is diabled Feature/lit 3911 add request id aggregation to logger when logging is disabled Sep 30, 2024
OFF = -1,
ERROR = 0,
INFO = 1,
DEBUG = 2,
Copy link
Author

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.

Copy link
Contributor

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?

Copy link
Contributor

@MaximusHaximus MaximusHaximus left a 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) {
Copy link
Contributor

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?

Copy link
Author

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.

@joshLong145 joshLong145 force-pushed the feature/lit-3911-add-request-id-aggregation-to-logger-when-logging-is-diabled branch from b66b0fd to f40c203 Compare October 2, 2024 12:59
}
}

return keys;
return keys
.sort((a: [string, number], b: [string, number]) => {
Copy link
Collaborator

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.

Copy link
Author

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.

@Ansonhkg Ansonhkg self-requested a review October 2, 2024 15:19
Copy link
Collaborator

@Ansonhkg Ansonhkg left a comment

Choose a reason for hiding this comment

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

LGTM!

@Ansonhkg Ansonhkg merged commit 17d6eb1 into master Oct 2, 2024
4 checks passed
@Ansonhkg Ansonhkg deleted the feature/lit-3911-add-request-id-aggregation-to-logger-when-logging-is-diabled branch October 2, 2024 15:22
@Ansonhkg Ansonhkg mentioned this pull request Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants