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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Oct 2, 2024
Merged

Feature/lit 3911 add request id aggregation to logger when logging is disabled #661

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
606840f
dev: allow request id aggregation if logging is disabled
Sep 30, 2024
2fcf3e7
chore: fmt
Sep 30, 2024
d0f3677
ref: change enum ordering
Sep 30, 2024
f40c203
Merge branch 'master' into feature/lit-3911-add-request-id-aggregatio…
Oct 1, 2024
09f2086
ref: remove checks on config flag
Oct 2, 2024
bd65454
ref: fix sort compareFn
Oct 2, 2024
47860e4
chore: fmt
Oct 2, 2024
5bbd27c
Merge branch 'master' into feature/lit-3911-add-request-id-aggregatio…
Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion packages/core/src/lib/lit-core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ import {
} from '@lit-protocol/types';

import { composeLitUrl } from './endpoint-version';
import { LogLevel } from '@lit-protocol/logger';

// eslint-disable-next-line @typescript-eslint/no-explicit-any
type Listener = (...args: any[]) => void;
Expand Down Expand Up @@ -202,7 +203,10 @@ export class LitCore {

// -- set global variables
globalThis.litConfig = this.config;
bootstrapLogManager('core');
bootstrapLogManager(
'core',
this.config.debug ? LogLevel.DEBUG : LogLevel.OFF
);

// -- configure local storage if not present
// LitNodeClientNodejs is a base for LitNodeClient
Expand Down Expand Up @@ -694,6 +698,7 @@ export class LitCore {
errorCode: LIT_ERROR.INIT_ERROR.name,
});
} catch (e) {
logErrorWithRequestId(requestId, e);
reject(e);
}
}, this.config.connectTimeout);
Expand Down
14 changes: 13 additions & 1 deletion packages/logger/src/lib/logger.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,18 @@ describe('logger', () => {
}

expect(lm.getLogsForId('foo7').length).toEqual(count);
expect(lm.LoggerIds.size).toEqual(1);
expect(lm.LoggerIds.length).toEqual(10);
});

it('should order logs based on logger creation timestamp', async () => {
const loggerA = lm.get('a', '1');
await new Promise((res) => setTimeout(res, 100));
const loggerB = lm.get('b', '2');

const requestIds = lm.LoggerIds;

expect(requestIds.length).toBe(2);
expect(loggerA.timestamp).toEqual(requestIds[0]);
expect(loggerB.timestamp).toEqual(requestIds[1]);
});
});
32 changes: 23 additions & 9 deletions packages/logger/src/lib/logger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@ import { hashMessage } from 'ethers/lib/utils';
import { version } from '@lit-protocol/constants';

export enum LogLevel {
INFO = 0,
DEBUG = 1,
WARN = 2,
ERROR = 3,
OFF = -1,
ERROR = 0,
INFO = 1,
DEBUG = 2,
Copy link
Author

@joshLong145 joshLong145 Sep 30, 2024

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

@FedericoAmura FedericoAmura Oct 2, 2024

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?

WARN = 3,
FATAL = 4,
TIMING_START = 5,
TIMING_END = 6,
OFF = -1,
}

const colours = {
Expand Down Expand Up @@ -207,6 +207,7 @@ export class Logger {
private _config: Record<string, any> | undefined;
private _isParent: boolean;
private _children: Map<string, Logger>;
private _timestamp: number;

public static createLogger(
category: string,
Expand All @@ -232,6 +233,7 @@ export class Logger {
this._config = config;
this._children = new Map();
this._isParent = isParent;
this._timestamp = Date.now();
}

get id(): string {
Expand All @@ -242,6 +244,10 @@ export class Logger {
return this._category;
}

get timestamp(): number {
return this._timestamp;
}

get Logs(): Log[] {
return this._logs;
}
Expand Down Expand Up @@ -312,13 +318,16 @@ export class Logger {
const arrayLog = log.toArray();
if (this._config?.['condenseLogs'] && !this._checkHash(log)) {
(this._level >= level || level === LogLevel.ERROR) &&
this._consoleHandler &&
this._consoleHandler(...arrayLog);
(this._level >= level || level === LogLevel.ERROR) &&
this._handler &&
this._handler(log);

(this._level >= level || level === LogLevel.ERROR) && this._addLog(log);
} else if (!this._config?.['condenseLogs']) {
(this._level >= level || level === LogLevel.ERROR) &&
this._consoleHandler &&
this._consoleHandler(...arrayLog);
(this._level >= level || level === LogLevel.ERROR) &&
this._handler &&
Expand All @@ -342,7 +351,6 @@ export class Logger {

private _addLog(log: Log) {
this._logs.push(log);

// TODO: currently we are not deleting old request id's which over time will fill local storage as the maximum storage size is 10mb
// we should be deleting keys from the front of the collection of `Object.keys(category)` such that the first keys entered are deleted when we reach a pre defined key threshold
// this implementation assumes that serialization / deserialization from `localStorage` keeps the same key ordering in each `category` object as we will asssume the array produced from `Object.keys` will always be the same ordering.
Expand Down Expand Up @@ -427,14 +435,20 @@ export class LogManager {
}

get LoggerIds(): string[] {
const keys: string[] = [];
const keys: [string, number][] = [];
for (const category of this._loggers.entries()) {
for (const child of category[1].Children) {
keys.push(child[0]);
keys.push([child[0], child[1].timestamp]);
}
}

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

@Ansonhkg Ansonhkg Oct 2, 2024

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

@joshLong145 joshLong145 Oct 2, 2024

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.

return a[1] > b[1] ? a[1] : b[1];
})
.map((value: [string, number]) => {
return value[0];
});
}

// if a logger is given an id it will persist logs under its logger instance
Expand Down
11 changes: 0 additions & 11 deletions packages/misc/src/lib/misc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,11 +279,6 @@ export const log = (...args: any): void => {
return;
}

if (globalThis?.litConfig?.debug !== true) {
Copy link
Contributor

@MaximusHaximus MaximusHaximus Oct 2, 2024

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

@joshLong145 joshLong145 Oct 2, 2024

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.

return;
}
// config is loaded, and debug is true

// if there are there are logs in buffer, print them first and empty the buffer.
while (logBuffer.length > 0) {
const log = logBuffer.shift() ?? '';
Expand All @@ -307,9 +302,6 @@ export const logWithRequestId = (id: string, ...args: any) => {
return;
}

if (globalThis?.litConfig?.debug !== true) {
return;
}
// config is loaded, and debug is true

// if there are there are logs in buffer, print them first and empty the buffer.
Expand Down Expand Up @@ -337,9 +329,6 @@ export const logErrorWithRequestId = (id: string, ...args: any) => {
return;
}

if (globalThis?.litConfig?.debug !== true) {
return;
}
// config is loaded, and debug is true

// if there are there are logs in buffer, print them first and empty the buffer.
Expand Down
Loading