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

feat(synth-bench): automate TPS measurement for wait_until: NONE #12874

Merged
merged 24 commits into from
Feb 14, 2025

Conversation

mooori
Copy link
Contributor

@mooori mooori commented Feb 4, 2025

As described in #12806, the RPC response to transactions sent with wait_until: NONE doesn't tell much. In that case, measuring TPS in terms of successfully processed transactions was a manual process. This PR automates it and uses that functionality for benchmarking native transfers.

The TransactionsStatisticsService added here can in the future also be used in other workloads.

Testing

  • Unit tests to verify metrics are extracted correctly from the report available under a nodes /metrics endpoint.
  • Running just benchmark_native_transfers prints TPS of successfully processed transactions.

Closes #12806

Copy link

codecov bot commented Feb 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.52%. Comparing base (8b3fc09) to head (47c110a).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12874      +/-   ##
==========================================
- Coverage   70.54%   70.52%   -0.02%     
==========================================
  Files         851      851              
  Lines      174983   174983              
  Branches   174983   174983              
==========================================
- Hits       123433   123407      -26     
- Misses      46419    46439      +20     
- Partials     5131     5137       +6     
Flag Coverage Δ
backward-compatibility 0.36% <ø> (ø)
db-migration 0.36% <ø> (ø)
genesis-check 1.42% <ø> (ø)
linux 70.36% <ø> (-0.01%) ⬇️
linux-nightly 70.16% <ø> (-0.01%) ⬇️
pytests 1.73% <ø> (ø)
sanity-checks 1.54% <ø> (ø)
unittests 70.35% <ø> (-0.02%) ⬇️
upgradability 0.36% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

impl MetricName {
fn report_name(self) -> &'static str {
match self {
MetricName::SuccessfulTransactions => "near_transaction_processed_successfully_total",
Copy link
Collaborator

Choose a reason for hiding this comment

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

One important thing to keep in mind is that this metric is incremented when the transaction successfully converts into a receipt. It does not mean that the receipt is actually getting executed (it might even happen in a different block if the receipt gets delayed.) You will probably want a metric along the lines of near_action_called_count (allows determining the rate of actions based on action types even in mixed action workloads, or where receipts have multiple actions!) or receipts (I don't think we have a single metric for all of them together, but shouldn't be too hard to add up the delayed, local, incoming, etc. receipts.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting! We've been using near_transaction_processed_successfully_total for that purpose since the ft benchmarking days...

Will look into near_action_called_count.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK this is currently the state of the art for determining successful txs in benchmarks. Usage of synth-bm is increasing, so I'd like to propose fixing this separately in order to merge this PR soon. Opened #12931.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if transaction_processed doesn't measure what it was expected to, it might still be useful to have support for that metric in the tooling. E.g. to check if all txs could successfully be converted to receipts.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be interested to know if transaction_processed is the best way to have right now to measure throughput , or if we should use / implement something else. Just asking because I've been using transaction_processed in my single and multi node measurements (only simple transfers , no complex operations)

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 think in your setup you're also getting numbers from /metrics and native transfers are a single action. So you could get the number of successful native transfers from the line in the /metrics response that looks like:

near_action_called_count{action="Transfer"} 200

(The line may be missing when no transfers were processed successfully yet.)

@mooori mooori requested review from Trisfald and nagisa February 14, 2025 11:38
@mooori mooori marked this pull request as ready for review February 14, 2025 11:38
@mooori mooori requested a review from a team as a code owner February 14, 2025 11:38
@mooori mooori requested a review from akhi3030 February 14, 2025 11:39
Copy link
Contributor

@Trisfald Trisfald left a comment

Choose a reason for hiding this comment

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

Overall LGTM, I only left a change request.

@@ -84,6 +85,11 @@ pub async fn benchmark(args: &BenchmarkArgs) -> anyhow::Result<()> {
rpc_response_handler.handle_all_responses().await;
});

let transaction_stat_service =
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a request: enable (or disable) transaction_stat_service through a command line flag.

The reason is that in my current workloads I don't always need this functionality. I could ignore the output, but I think it's cleaner to have a flag to turn this off

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@mooori mooori requested a review from Trisfald February 14, 2025 15:44
@mooori mooori enabled auto-merge February 14, 2025 15:49
@mooori mooori added this pull request to the merge queue Feb 14, 2025
Merged via the queue into near:master with commit eeb5b9f Feb 14, 2025
29 checks passed
@mooori mooori deleted the bm-measure-tps branch February 14, 2025 16:18
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.

synth-bench: automate TPS measurement for wait_until: NONE
3 participants