Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

RPC getPerformanceSamples: Add numNonVoteTransaction #29388

Merged
merged 1 commit into from
Jan 18, 2023
Merged

RPC getPerformanceSamples: Add numNonVoteTransaction #29388

merged 1 commit into from
Jan 18, 2023

Conversation

ilya-bobyr
Copy link
Contributor

Problem

Allow interested parties to see both total and non-vote transaction counts in each performance sample.

Summary of Changes

  • banks now count non-vote transactions.
  • blockstore stores those counts into a new version of the PerfSample column.
  • SamplePerformanceService pushes new count from the banks into the blockstore.
  • RPC reports the new count, when it is available for the slot.

Fixes #29159

@ilya-bobyr ilya-bobyr mentioned this pull request Dec 23, 2022
Closed
@ilya-bobyr ilya-bobyr requested review from CriesofCarrots, yhchiang-sol and mvines and removed request for yhchiang-sol December 23, 2022 09:31
@ilya-bobyr ilya-bobyr marked this pull request as ready for review December 23, 2022 09:32
@ilya-bobyr ilya-bobyr requested review from steviez and removed request for mvines December 23, 2022 09:45
CriesofCarrots
CriesofCarrots reviewed Jan 13, 2023
View reviewed changes
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

I love how simple this one turned out :)
Just a couple suggestions on your tests.

docs/src/developing/clients/jsonrpc-api.md Show resolved Hide resolved
rpc-client-api/src/response.rs Outdated Show resolved Hide resolved
rpc-client-api/src/response.rs Outdated Show resolved Hide resolved
rpc-client-api/src/response.rs Outdated Show resolved Hide resolved
rpc-client-api/src/response.rs Outdated Show resolved Hide resolved
rpc-client-api/src/response.rs Outdated Show resolved Hide resolved
rpc-client-api/src/response.rs Show resolved Hide resolved
rpc-client-api/src/response.rs Outdated Show resolved Hide resolved
rpc-client-api/src/response.rs Outdated Show resolved Hide resolved
ilya-bobyr
ilya-bobyr commented Jan 13, 2023
View reviewed changes
docs/src/developing/clients/jsonrpc-api.md Outdated
@@ -2074,6 +2079,7 @@ An array of:
- `RpcPerfSample<object>`
- `slot: <u64>` - Slot in which sample was taken at
- `numTransactions: <u64>` - Number of transactions in sample
- `numNonVoteTransaction: <u64>` - Number of non-vote transactions in sample
Copy link
Contributor Author

@ilya-bobyr ilya-bobyr Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Now I wonder if I should clarify numNonVoteTransaction with an explanation that it is only present after upgrade to v1.15 and only for newer blocks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I was going to say that if we do document the version added, the public JSON-RPC docs are probably the place to do it. Historically, we've only done that for input parameters, not new output fields. But we could start.

Copy link
Contributor

Choose a reason for hiding this comment

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

We do have some precedent of doing so for new methods:

solana/docs/src/developing/clients/jsonrpc-api.md

Line 1056 in 055c6a5

**NEW: This method is only available in solana-core v1.9 or newer. Please use

CriesofCarrots
CriesofCarrots approved these changes Jan 13, 2023
View reviewed changes
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment •
edited
Loading

Choose a reason for hiding this comment

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

r+ a nit, CI's complaints, and whatever you decide on the jsonrpc-api docs

docs/src/developing/clients/jsonrpc-api.md Outdated
@@ -2074,6 +2079,7 @@ An array of:
- `RpcPerfSample<object>`
- `slot: <u64>` - Slot in which sample was taken at
- `numTransactions: <u64>` - Number of transactions in sample
- `numNonVoteTransaction: <u64>` - Number of non-vote transactions in sample
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I was going to say that if we do document the version added, the public JSON-RPC docs are probably the place to do it. Historically, we've only done that for input parameters, not new output fields. But we could start.

rpc-client-api/src/response.rs Outdated Show resolved Hide resolved
steviez
steviez reviewed Jan 13, 2023
View reviewed changes
Copy link
Contributor

@steviez steviez left a comment

Choose a reason for hiding this comment

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

A couple small things but agreed with Tyera that is nice how clean / minimal this PR is

docs/src/developing/clients/jsonrpc-api.md Outdated
@@ -2074,6 +2079,7 @@ An array of:
- `RpcPerfSample<object>`
- `slot: <u64>` - Slot in which sample was taken at
- `numTransactions: <u64>` - Number of transactions in sample
- `numNonVoteTransaction: <u64>` - Number of non-vote transactions in sample
Copy link
Contributor

Choose a reason for hiding this comment

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

We do have some precedent of doing so for new methods:

solana/docs/src/developing/clients/jsonrpc-api.md

Line 1056 in 055c6a5

**NEW: This method is only available in solana-core v1.9 or newer. Please use

docs/src/developing/clients/jsonrpc-api.md Outdated Show resolved Hide resolved
rpc-client-api/src/response.rs Outdated Show resolved Hide resolved
CriesofCarrots
CriesofCarrots previously approved these changes Jan 13, 2023
View reviewed changes
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Erm, this one comment didn't post with my review. Not sure why. I'm having weird internet stuff at home, but also git seems sluggish independent of that 🤷‍♀️.

rpc-client-api/src/response.rs Outdated Show resolved Hide resolved
@mergify mergify bot dismissed CriesofCarrots’s stale review January 14, 2023 03:37

Pull request has been modified.

CriesofCarrots
CriesofCarrots previously approved these changes Jan 15, 2023
View reviewed changes
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

r+ one remaining nit

rpc-client-api/src/response.rs Outdated Show resolved Hide resolved
steviez
steviez previously approved these changes Jan 16, 2023
View reviewed changes
@mergify mergify bot dismissed stale reviews from CriesofCarrots and steviez January 18, 2023 00:10

Pull request has been modified.

@ilya-bobyr
RPC getPerformanceSamples: Add numNonVoteTransaction
283179e 
Allow interested parties to see both total and non-vote transaction
counts in each performance sample.
@ilya-bobyr ilya-bobyr merged commit 6e4ecc6 into solana-labs:master Jan 18, 2023
@ilya-bobyr ilya-bobyr deleted the getRecentPerformanceSamples-split-votes branch January 18, 2023 08:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@steviez steviez steviez left review comments

@CriesofCarrots CriesofCarrots CriesofCarrots left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make getRecentPerformanceSamples() return the number of vote and non-vote transactions
3 participants
@ilya-bobyr @CriesofCarrots @steviez