Skip to content
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

improve(relayer): Track transaction submission time #2093

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

pxrl
Copy link
Contributor

@pxrl pxrl commented Feb 13, 2025

In order to better understand transaction submission delays, monitor and report on the time required for transaction submission to occur on each chain.

In order to better understand transaction submission delays, monitor and
report on the time required for transaction submission to occur on each
chain.
pendingTxnReceipts[chainId] = multiCallerClient.executeTxnQueue(chainId, simulate);
const txnReceipts = await pendingTxnReceipts[chainId];
profiler.stop({ message: `Completed ${chain} transaction submission.`, txnReceipts });
Copy link
Member

Choose a reason for hiding this comment

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

thoughts on logging this in the fill info log we send to the SlackTransport?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realised this is very close to the log message we already emit in TransactionUtils, so I relocated the change down there. It wasn't clear for me how I should modify the Profiler to support logging at level info, so for now I've just manually added the necessary fields to keep it simple, and also because I don't want to go yak shaving in the SDK just to get this in, because this is more important. wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's an example from local test:

2025-02-14 13:16:16 [info]: {
  "at": "TransactionClient#submit",
  "message": "Completed Arbitrum One transaction submission! 🧙",
  "mrkdwn": "  1. Filled v3 deposit 🚀 (<https://arbiscan.io/tx/0x9896e8bc9233e638de651f86476ba39466bd53770de6427a7ac5cd1974f55ddb|0x9..ddb>): Relayed depositId 3183273 from Base to Arbitrum One of 1.00 USDC and output 0.9881 USDC, with depositor <https://basescan.org/address/0x9A8f92a830A5cB89a3816e3D267CB7791c16b04D|0x9..04D>. Realized LP fee: 0.000%, total fee: 1.18%. Gas price used in profit calc: 0.01000 Gwei. Relayer repayment: Arbitrum One.\n",
  "duration": 744,
  "chain": "Arbitrum One",
  "datadog": true,
  "bot-identifier": "sky",
  "run-identifier": "c3a8bbaa-720d-43df-bd64-8325d027e800"
}

nicholaspai
nicholaspai previously approved these changes Feb 13, 2025
pendingTxnReceipts[chainId] = multiCallerClient.executeTxnQueue(chainId, simulate);
const txnReceipts = await pendingTxnReceipts[chainId];
profiler.stop({ message: `Completed ${chain} transaction submission.`, txnReceipts });
Copy link
Member

Choose a reason for hiding this comment

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

another idea is this time should maybe be dependent on the # of txns in the queue or the calldata size. Thoughts on incorporating that into the log somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I did wonder about that, since we can have multiple txns here. You can see it based on the length of the txnReceipts array, but I'll probably add a specific entry for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nicholaspai
nicholaspai previously approved these changes Feb 13, 2025
@pxrl pxrl requested a review from nicholaspai February 14, 2025 12:29
this.logger.info({
at: "TransactionClient#submit",
message: `Completed ${chain} transaction submission! 🧙`,
mrkdwn,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mrkdwn should be set on each loop, rather than being appended-to.

Copy link
Contributor

@bmzig bmzig Feb 18, 2025

Choose a reason for hiding this comment

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

Maybe I'm misunderstanding this, but won't this log just be emitted for every transaction we submit? So if we have two transactions: [txn1, txn2] then this will log twice, the first log being:

completed [chain] transaction submission
... txn1 details

on the first iteration of the loop and then

completed [chain] transaction submission
... txn1 details
... txn2 details

on the second iteration? (Since we are still doing mkdwn +=)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly - this is not what we want. I left the comment to earmark the change but haven't implemented it yet 👍

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.

3 participants