-
Notifications
You must be signed in to change notification settings - Fork 44
[DNM] Add logOperationTime util with implementation in protocol module #398
base: main
Are you sure you want to change the base?
Conversation
@@ -23,6 +23,5 @@ | |||
"eslint-plugin-import": "2.22.1", | |||
"eslint-plugin-prettier": "3.1.4", | |||
"pino-pretty": "4.3.0" | |||
}, | |||
"dependencies": {} | |||
} |
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.
Not sure why this is getting removed for me - might be result of using later version of node. Can fix before I check in. Likely above package-lock.json diff is for same reason.
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.
Not really an issue, we can push this change. We will be updating to Node 14 soon anyways.
const stopLabel = `${label}-stop`; | ||
const timestamp = performance.mark(stopLabel); | ||
const diff = performance.measure(method, label, stopLabel); | ||
const message = `${method} : finished @ ${timestamp}; took ${diff} ms`; |
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.
Probably only need the diff here since pino adds timestamps
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.
Yep, this is from earlier iteration, good catch
/// time method took to execute in ms). | ||
export const logOperationTime = (log: pino.BaseLogger, method: string, label?: string): string => { | ||
if (label) { | ||
const stopLabel = `${label}-stop`; |
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.
What would happen if there are label collisions?
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.
Currently using a unique identifier below as part of generated label to prevent that, but this will be replaced by passed in methodId
EDIT: But to answer question, yeah, if there are label collisions, it would overwrite the prev mark. Which would be bad, bc an async method of same name could be called twice
@@ -199,7 +201,7 @@ export class Vector implements IVectorProtocol { | |||
} | |||
const method = "onReceiveProtocolMessage"; | |||
const methodId = getRandomBytes32(); | |||
this.logger.debug({ method, methodId }, "Method start"); | |||
const label = logOperationTime(this.logger, method); |
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.
We will likely want to keep the methodId
to log throughout the other parts of the method so the logs are easily searchable. Maybe we should use the label
as the methodId
instead.
General purpose of the methodId
is to get a unique strings so that you can easily identify all logs that came from a given method when searching in log dna or whatever other log parser
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.
Sure. This functionality is already internal to the logOperationTime
method. I actually had it like that at first, but worried that not all methods would follow this standard of generating a methodId. Will add this back tho, thanks
We should make this time logging part of the "debug" log config. IMO it should be in the code and be able to be enabled with a "switch flip" of sorts. |
Can you clarify what you mean by "debug" log config? Is there a configuration somewhere I should be aware of? The way it is currently, unless the method takes an inordinate amount of time to execute, it will always log it on debug level. Takes too long and it logs it as info, then warning |
https://docs.connext.network/configuring-a-router here's the config. Your implementation sounds good. The main thing I was trying to get at is we should have the ability to turn this on/off in the config. So as part of this PR I think we should add a config item called |
Ah, I misread your first comment. I thought you said it should NOT be able to be enabled by switch flip of sorts , haha. Yes, agreed. I will target that functionality for this PR, thanks. (Meaning I will include addition of this config item) EDIT: I'm going to go out on a limb and assume the responsibility to 'check' the switch should be relegated to the method itself (meaning I'll check the |
The Problem
The Solution