Skip to content

refactor: make oof metric more generic to follow the standard - #110

Merged
kirtan-amin merged 1 commit into
mainfrom
chore/CCIP-11890-oof-metric-standard
Jul 14, 2026
Merged

refactor: make oof metric more generic to follow the standard#110
kirtan-amin merged 1 commit into
mainfrom
chore/CCIP-11890-oof-metric-standard

Conversation

@kirtan-amin

@kirtan-amin kirtan-amin commented Jul 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Update the insufficient funds metric to be more generic by using the cause label pattern that is used for our currently deployed metrics. This also removes fromAddress due to it possible having a high cardinality in the future.
CCIP-11890

Requires Dependencies

Resolves Dependencies

@kirtan-amin
kirtan-amin requested a review from a team as a code owner July 14, 2026 14:13
@github-actions

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

👋 kirtan-amin, thanks for creating this pull request!

To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team.

Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks!

@github-actions

github-actions Bot commented Jul 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

📊 API Diff Results

No changes detected for module github.com/smartcontractkit/chainlink-framework/metrics

View full report

@kirtan-amin
kirtan-amin force-pushed the chore/CCIP-11890-oof-metric-standard branch 2 times, most recently from 71cf95c to 7cdfda3 Compare July 14, 2026 14:44
@github-actions

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

⚠️ API Diff Results - github.com/smartcontractkit/chainlink-framework/chains

⚠️ Breaking Changes (2)

txmgr.broadcasterMetrics (1)
  • IncrementNumInsufficientFundsForTx — Type changed:
func(
  - context.Context, 
  - string
  + context.Context
)
txmgr.confimerMetrics (1)
  • IncrementNumInsufficientFundsForTx — Type changed:
func(
  - context.Context, 
  - string
  + context.Context
)

📄 View full apidiff report

bukata-sa
bukata-sa previously approved these changes Jul 14, 2026
View reviewed changes
@kirtan-amin
kirtan-amin enabled auto-merge (squash) July 14, 2026 16:07
amit-momin
amit-momin reviewed Jul 14, 2026
View reviewed changes
Comment thread metrics/txm.go
amit-momin
amit-momin reviewed Jul 14, 2026
View reviewed changes
Comment thread metrics/txm.go Outdated
}, []string{"chainID", "senderAddress"})
promAttemptError = promauto.NewCounterVec(prometheus.CounterOpts{
Name: "tx_manager_attempt_error_total",
Help: "Number of transaction broadcast/confirm attempts that failed, labelled by cause. For cause=\"insufficient_funds\" the sending address was rejected by an RPC node for insufficient funds; it increments on every retry while the address remains underfunded, so a sustained rate indicates an address needs topping up. The sending address is intentionally not a label (unbounded cardinality) - find it via the fromAddress key in the TXM warn log.",

@amit-momin amit-momin Jul 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it really unbounded cardinality? The address is pretty much set per chain per node. It's not like an execution ID that changes every time. The reason it's helpful here is because getting info from logs assumes we have log streams enabled across all products and all NOPs. If we don't, we won't have access to the logs to get this address from. We could technically get it from the node_balance metric instead but this would imply we also have the issue with cardinality on that metric as well

@kirtan-amin kirtan-amin Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this was my initial thought but @bukata-sa suggested we might run into issues due to possible high cardinality now / in the future. By the sounds of it this isn't an issue so if you are happy we can add it back in?

@amit-momin amit-momin Jul 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ya if the node_balance metric is any indicator this should be fine keep as a label

@kirtan-amin kirtan-amin Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok cool, pushed the changes

@kirtan-amin
kirtan-amin force-pushed the chore/CCIP-11890-oof-metric-standard branch from 7cdfda3 to f334ba0 Compare July 14, 2026 19:29
@kirtan-amin
kirtan-amin requested review from silaslenihan and removed request for ilija42 July 14, 2026 19:37
@kirtan-amin
kirtan-amin merged commit 527495d into main Jul 14, 2026
25 checks passed
@kirtan-amin
kirtan-amin deleted the chore/CCIP-11890-oof-metric-standard branch July 14, 2026 19:40
@kirtan-amin kirtan-amin mentioned this pull request Jul 16, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Unheilbar Unheilbar Unheilbar approved these changes
@amit-momin amit-momin amit-momin approved these changes
@bukata-sa bukata-sa bukata-sa left review comments
@silaslenihan silaslenihan Awaiting requested review from silaslenihan

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kirtan-amin @Unheilbar @bukata-sa @amit-momin