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

Expose transaction count by type metrics for the layered txpool #6903

Merged
merged 2 commits into from
Apr 8, 2024

Conversation

fab-10
Copy link
Contributor

@fab-10 fab-10 commented Apr 8, 2024

PR description

Expose transaction count by type metrics for each layer of the layered txpool

The new metric name is besu_transaction_pool_number_of_transactions_by_type and has layer and type as labels.

Example panel
image

Besu Full Grafana dashboard will be updated to render this metrics once the PR will be merged.

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

  • unit tests: ./gradlew build
  • acceptance tests: ./gradlew acceptanceTest
  • integration tests: ./gradlew integrationTest
  • reference tests: ./gradlew ethereum:referenceTests:referenceTests

fab-10 added 2 commits April 8, 2024 14:56
Signed-off-by: Fabio Di Fabio <[email protected]>
@fab-10 fab-10 force-pushed the layered-txpool-tx-type-metric branch from 389a174 to 6257d7f Compare April 8, 2024 12:56
@fab-10 fab-10 marked this pull request as ready for review April 8, 2024 13:00
@fab-10 fab-10 changed the title Add tx count by type txpool metric Expose transaction count by type metrics for the layered txpool Apr 8, 2024
@fab-10 fab-10 enabled auto-merge (squash) April 8, 2024 14:01
.forEach(
type ->
metrics.initTransactionCountByType(
() -> txCountByType[type.ordinal()], name(), type));
Copy link
Contributor

Choose a reason for hiding this comment

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

using ordinal to track the external feels fragile, but if experimental tx types start showing up they may leave large gaps. However... it's what we express externally, so this looks like it will work. See my other comment on the metric.

"number_of_transactions_by_type",
"The number of transactions, of a specified type, currently present in the layer",
"layer",
"type");
Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to the type name, should we expose the EIP-2718 type number? We shouldn't use ordinal and should encode it in TransactionType but that has some weirdness for frontier types. May be best for a future PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I think it makes sense to explicitly add the EIP-2718 type number to the tx type, and expose it in the metric label.
About the use of type.ordinal(), it is only used internally to index the array of int for the counters, and not exposed outside.

@fab-10 fab-10 merged commit e8e1e4c into hyperledger:main Apr 8, 2024
42 checks passed
@fab-10 fab-10 deleted the layered-txpool-tx-type-metric branch April 8, 2024 15:40
amsmota pushed a commit to Citi/besu that referenced this pull request Apr 16, 2024
amsmota pushed a commit to Citi/besu that referenced this pull request Apr 16, 2024
macfarla pushed a commit to macfarla/besu that referenced this pull request Apr 26, 2024
matthew1001 pushed a commit to kaleido-io/besu that referenced this pull request Jun 7, 2024
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.

2 participants