Implement log tagging for SDK logs #69
Conversation
gasperzgonec
requested review from
a team,
patricijabrecko and
radovanjorgic
as code owners
gasperzgonec
force-pushed
the
gasperz/ISS202822
branch
from
1dcaaca to
9cd8095
Compare
There was a problem hiding this comment.
TBH I don't understand this whole complexity behind. So, what we want achieve is just prefixing SDK logs with [SDK] or sth like that, and possibly user logs with [USER]. Most likely I don't understand security issues here, but seems a bit like an overkill to me and sth that can overcomplicate our lifes. Let's discuss this on call.
gasperzgonec
force-pushed
the
gasperz/ISS202822
branch
2 times, most recently
from
6c7d0fc to
51e57d8
Compare
Added SDK prefix ('SDK: ') to all logs coming from the SDK itself.
The class 'sdkconsole' isn't exported and so it shouldn't be possible to
generate it.
It's a convenience class, so that I was able to replace console logs
with minimal changes.
Split the communication into two channels, one for "SDK" and one for the 3rd party user called "USER". The channel name is written in front of the message (e.g: `[SDK] Uploading users`). The SDK needs to do additional steps to use the SDK channel, but 3rd party users don't.
gasperzgonec
force-pushed
the
gasperz/ISS202822
branch
from
52f571f to
87a94c1
Compare
There was a problem hiding this comment.
I added few suggestions, will probably add more comments in second review. However, since logger overrides console, can we use console.log/error/warn... at all other places, let's have clear distinction here?
Also, let's ask @navneel99 for quick review after tomorrow's meeting.
src/logger/logger.ts
Outdated
| export class Logger extends Console { | ||
| private options?: WorkerAdapterOptions; | ||
| private tags: EventContext & { dev_oid: string }; | ||
| private isVerifiedChannel: boolean = false; // false = unverified (default), true = verified |
There was a problem hiding this comment.
isSdkChannel rather than verified?
src/logger/logger.ts
Outdated
|
|
||
| const logObject = { | ||
| message, | ||
| verified: this.isVerifiedChannel, |
There was a problem hiding this comment.
Can we add this to tags variable?
| super(process.stdout, process.stderr); | ||
| this.options = options; | ||
| this.tags = { | ||
| ...event.payload.event_context, |
There was a problem hiding this comment.
Let's add verified here and we rather have something like sdk_log? verified is not self-explainatory in my opinion.
There was a problem hiding this comment.
I've renamed the verified to sdk_log and moved it to tags.
| options, | ||
| }: SpawnFactoryInterface<ConnectorState>): Promise<void> { | ||
| const logger = new Logger({ event, options }); | ||
| const logger = getInternalLogger(new Logger({ event, options })); |
There was a problem hiding this comment.
Can we rather have factory function for this class (e.g. createLogger(isSdkLogger: boolean)?
There was a problem hiding this comment.
I'm not entirely sure how to implement that.
createLogger(isSdkLogger: boolean, logger: Logger)
or
createLogger(isSdkLogger: boolean, event: EventContext, options: any)?
There was a problem hiding this comment.
Like this?
createLogger(isSdkLogger: boolean, event: AirdropEvent, options: any)
Description
This PR changes how the SDK internal logs are printed. As it currently stands, the SDK logs are all prefixed with
SDK:.Connected Issues
Checklist
npm run testOR no tests needed.npm run test:backwards-compatibility.