Skip to content
This repository has been archived by the owner on Dec 27, 2022. It is now read-only.

[DNM] Add logOperationTime util with implementation in protocol module #398

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
13 changes: 8 additions & 5 deletions modules/protocol/src/vector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
ProtocolError,
jsonifyError,
} from "@connext/vector-types";
import { getCreate2MultisigAddress, getRandomBytes32 } from "@connext/vector-utils";
import { getCreate2MultisigAddress, getRandomBytes32, logOperationTime } from "@connext/vector-utils";
import { Evt } from "evt";
import pino from "pino";

Expand Down Expand Up @@ -132,6 +132,7 @@ export class Vector implements IVectorProtocol {
): Promise<Result<FullChannelState, OutboundChannelUpdateError>> {
const method = "executeUpdate";
const methodId = getRandomBytes32();
const label = logOperationTime(this.logger, method);
this.logger.debug({
method,
methodId,
Expand Down Expand Up @@ -178,6 +179,7 @@ export class Vector implements IVectorProtocol {
);
}

logOperationTime(this.logger, method, label);
return outboundRes;
}

Expand All @@ -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);
Copy link
Contributor

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

Copy link
Contributor Author

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


if (msg.isError) {
this.logger.error(
Expand Down Expand Up @@ -267,7 +269,7 @@ export class Vector implements IVectorProtocol {
updatedTransfers: updatedActiveTransfers,
updatedTransfer,
});
this.logger.debug({ method, methodId }, "Method complete");
logOperationTime(this.logger, method, label);
},
);

Expand Down Expand Up @@ -355,7 +357,7 @@ export class Vector implements IVectorProtocol {
public async setup(params: ProtocolParams.Setup): Promise<Result<FullChannelState, OutboundChannelUpdateError>> {
const method = "setup";
const methodId = getRandomBytes32();
this.logger.debug({ method, methodId }, "Method start");
const label = logOperationTime(this.logger, method);
// Validate all parameters
const error = this.validateParamSchema(params, ProtocolParams.SetupSchema);
if (error) {
Expand Down Expand Up @@ -400,14 +402,15 @@ export class Vector implements IVectorProtocol {
},
"Method complete",
);
logOperationTime(this.logger, method, label);
return returnVal;
}

// Adds a deposit that has *already occurred* onchain into the multisig
public async deposit(params: ProtocolParams.Deposit): Promise<Result<FullChannelState, OutboundChannelUpdateError>> {
const method = "deposit";
const methodId = getRandomBytes32();
this.logger.debug({ method, methodId }, "Method start");
const label = logOperationTime(this.logger, method);
// Validate all input
const error = this.validateParamSchema(params, ProtocolParams.DepositSchema);
if (error) {
Expand Down
33 changes: 33 additions & 0 deletions modules/utils/src/logOperationTime.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import pino from "pino";
import { performance } from 'perf_hooks';
import { getRandomBytes32 } from "@connext/vector-utils";

/// This method is used to log the start and end time of an executed method. We are using the
/// perf_hooks (performance) lib provided by node to calculate this - much more accurate than
/// Date.now(). On first call, a unique label string is generated and returned. On second call,
/// same label string should be passed in to measure and log the diff in time (i.e. amount of
/// time method took to execute in ms).
export const logOperationTime = (log: pino.BaseLogger, method: string, label?: string): string => {
if (label) {
const stopLabel = `${label}-stop`;
Copy link
Contributor

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?

Copy link
Contributor Author

@jakekidd jakekidd Feb 24, 2021

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

const timestamp = performance.mark(stopLabel);
const diff = performance.measure(method, label, stopLabel);
const message = `${method} : finished @ ${timestamp}; took ${diff} ms`;
Copy link
Contributor

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

Copy link
Contributor Author

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

// Depending on how long method took, up severity of logging call. This is a default
// arrangement that was copied over from indra.
if (diff < 2) {
log.debug(message);
} else if (diff < 200) {
log.info(message);
} else {
log.warn(message);
}
} else {
// Generate the label we'll be using (for later ref) using unique hexstring.
const label = `${method}-${getRandomBytes32()}`;
performance.mark(label);
const message = `${method}: started`;
log.debug(message);
}
return label;
};
Loading