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.

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

Count eth transactions and errors #1051

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

Count eth transactions and errors #1051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions src/services/blockchain/ethereum/ethereum-blockchain-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ import {
TransactionResponse,
TransactionReceipt,
} from '@ethersproject/abstract-provider'
import { ServiceMetrics as Metrics } from '@ceramicnetwork/observability'
import { Utils } from '../../../utils.js'
import { METRIC_NAMES } from '../../../settings.js'

const BASE_CHAIN_ID = 'eip155'
const TX_FAILURE = 0
Expand Down Expand Up @@ -410,6 +412,7 @@ export class EthereumBlockchainService implements BlockchainService {
const txData = await this._buildTransactionRequest(rootCid)
const txResponses: Array<TransactionResponse> = []

Metrics.count(METRIC_NAMES.ETH_REQUEST_TOTAL, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this metric represent?

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 its just for comparing to the error total - its like how many times did we make an eth transaction request, vs how many times it errored

it might also be the same as the number of anchors but unless we have knowledge of the code we don't know that for sure

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes currently this should corresponding to the number of anchors. Do you want this to include the number of retries? If so then you should move it into the try block after _trySendTransaction.

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 don't think i want to move it to include retries, its just for the purpose of comparing to errors. we could remove it entirely and just count the errors maybe

Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think i want to move it to include retries, its just for the purpose of comparing to errors. we could remove it entirely and just count the errors maybe

I think that makes sense

return this.withWalletBalance((walletBalance) => {
return attempt(MAX_RETRIES, async (attemptNum) => {
try {
Expand All @@ -420,6 +423,7 @@ export class EthereumBlockchainService implements BlockchainService {
} catch (err: any) {
logger.err(err)
const { code } = err
Metrics.count(METRIC_NAMES.ETH_REQUEST_ERROR_TOTAL, 1, {'error_code': code})
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a case where this "should" fail. Ex. First attempt is successful but times out before it can return success. Second attempt fails because the first attempt was successful (the error is nonce expired). Instead of throwing the error we check if the first attempt was successful.

If this count disregards the above scenario then you can also disregard this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok yeah that would change the behavior aside from metrics - i guess i will defer this or icebox for now, bc i think changing the error behavior of cas is out of scope from measuring the things

The code should have something to do with the reason but not sure if it will sort these...

switch (code) {
case ErrorCode.INSUFFICIENT_FUNDS:
return handleInsufficientFundsError(txData, walletBalance)
Expand Down
6 changes: 6 additions & 0 deletions src/settings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@ export enum METRIC_NAMES {
ANCHOR_REQUESTS_BATCH_TIME = 'anchor_requests_batch_time',
ANCHOR_REQUESTS_BATCH_FAILURE_TIME = 'anchor_requests_batch_failure_time',

// *******************************************************************//
// Ethereum Blockchain Service

ETH_REQUEST_TOTAL = 'eth_request_total',
ETH_REQUEST_ERROR_TOTAL = 'eth_request_error_total',

// *******************************************************************//
// Scheduler Service
SCHEDULER_TASK_UNCAUGHT_ERROR = 'scheduler_task_uncaught_error',
Expand Down