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

metrics: add counters to broadcastSignedTxGroup #5588

Merged
merged 1 commit into from Aug 2, 2023

Conversation

cce
Copy link
Contributor

@cce cce commented Jul 20, 2023

Summary

Signed transactions intended for broadcast from a node (via the REST API) go through this broadcastSignedTxGroup method for validation first. This adds counters to track how often they succeed vs fail.

Test Plan

Existing tests should pass.

@cce cce force-pushed the count-txgroup-broadcast-errors branch from c401489 to 394adc1 Compare July 20, 2023 17:07
@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Merging #5588 (394adc1) into master (d316914) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #5588      +/-   ##
==========================================
- Coverage   55.79%   55.78%   -0.01%     
==========================================
  Files         446      446              
  Lines       63418    63423       +5     
==========================================
- Hits        35381    35378       -3     
- Misses      25668    25673       +5     
- Partials     2369     2372       +3     
Impacted Files Coverage Δ
node/node.go 4.16% <0.00%> (-0.03%) ⬇️

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cce cce marked this pull request as ready for review July 24, 2023 17:53
Copy link
Contributor

@iansuvak iansuvak left a comment

Choose a reason for hiding this comment

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

Generally LGTM, the only comment is around the naming of the metrics. The PR explains that the benefit of these is that we get to count successes/failures of broadcasting messages received via REST API specifically but nothing else about their names or about the method suggests that this codepath is only used for txns received via REST.

Not a blocker either way since people can trace through the code and see it or find the PR but just digging through a list of /metrics might be non-intuitive.

@algorandskiy
Copy link
Contributor

Not a blocker either way since people can trace through the code and see it or find the PR but just digging through a list of /metrics might be non-intuitive.

I agree, metric names have to be re-unified. Maybe make a cleanup by introducing new names and then retiring the old ones in next release

@algorandskiy algorandskiy merged commit ea9efcd into algorand:master Aug 2, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants