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.

Sign up for GitHub

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

Jump to bottom

Add a metric for filtered txs #2517

Closed
evan-forbes opened this issue Sep 17, 2023 · 0 comments · Fixed by #2560
Closed

Add a metric for filtered txs #2517

evan-forbes opened this issue Sep 17, 2023 · 0 comments · Fixed by #2560
Assignees
@cmwaters

Comments

@evan-forbes
Copy link
Member

evan-forbes commented Sep 17, 2023 •
edited
Loading

While there are currently known exceptions to this rule due to CheckTx not ordering blob txs before normal ones #1947, in general all transactions that make it through CheckTx should also be able to make it through PrepareProposal. Therefore we should keep track of when transactions get filtered in PrepareProposal.

As suggested by by @cmwaters, we should monitor the filtered txs via metrics and and logged errors. We already have access to the logger where txs are filtered in the app, but the application currently does not have access to metrics.

Perhap not an exaustive list, but to fix this we could:

  • return the metrics that we want to record in the Prepare/ProcessProposalResponses
  • initialize the application with its own prometheus metrics (its own client and port)
  • pass the existing comet prometheus client to the application
  • query the application via a new abci query at specific points
@evan-forbes evan-forbes mentioned this issue Sep 18, 2023
Merged
5 tasks
evan-forbes added a commit that referenced this issue Sep 19, 2023
@evan-forbes
chore: log filtered transactions and fix gas limits in tests (#2518)
dd50ae7 
## Overview

This PR adds loggers back to the filter functions (but actually uses
them this time), and logs when transaction get filtered during
`PrepareProposal`. It also re-adds an obtuse gas limit to the
transactions in the `TestPrepareProposalConsistency` test, where those
errors were getting caught by the filter functions.

part of #2517

## Checklist

- [x] New and updated code has appropriate documentation
- [x] New and updated code has new and/or updated testing
- [x] Required CI checks are passing
- [x] Visual proof for any user facing features like CLI or
documentation updates
- [x] Linked issues closed with keywords
mergify bot pushed a commit that referenced this issue Sep 19, 2023
@evan-forbes @mergify
chore: log filtered transactions and fix gas limits in tests (#2518)
028d39c 
## Overview

This PR adds loggers back to the filter functions (but actually uses
them this time), and logs when transaction get filtered during
`PrepareProposal`. It also re-adds an obtuse gas limit to the
transactions in the `TestPrepareProposalConsistency` test, where those
errors were getting caught by the filter functions.

part of #2517

## Checklist

- [x] New and updated code has appropriate documentation
- [x] New and updated code has new and/or updated testing
- [x] Required CI checks are passing
- [x] Visual proof for any user facing features like CLI or
documentation updates
- [x] Linked issues closed with keywords

(cherry picked from commit dd50ae7)

# Conflicts:
#	app/test/fuzz_abci_test.go
@cmwaters cmwaters self-assigned this Sep 21, 2023
@cmwaters cmwaters mentioned this issue Sep 21, 2023
Merged
cmwaters added a commit that referenced this issue Sep 21, 2023
@cmwaters
feat: add metrics to prepare and process proposal (#2560)
5682d2c 
Closes: #2517

This PR adds metrics to track:
- The amount of times process proposal recovers from a panic
- The amount of invalid transactions that get filtered in prepare
proposal
- The time it takes for prepare proposal and process proposal to execute
mergify bot pushed a commit that referenced this issue Sep 21, 2023
@cmwaters @mergify
feat: add metrics to prepare and process proposal (#2560)
6551c6c 
Closes: #2517

This PR adds metrics to track:
- The amount of times process proposal recovers from a panic
- The amount of invalid transactions that get filtered in prepare
proposal
- The time it takes for prepare proposal and process proposal to execute

(cherry picked from commit 5682d2c)
0xchainlover pushed a commit to celestia-org/celestia-app that referenced this issue Aug 1, 2024
@cmwaters
feat: add metrics to prepare and process proposal (#2560)
9d6ac4d 
Closes: celestiaorg/celestia-app#2517

This PR adds metrics to track:
- The amount of times process proposal recovers from a panic
- The amount of invalid transactions that get filtered in prepare
proposal
- The time it takes for prepare proposal and process proposal to execute
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cmwaters cmwaters

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: add metrics to prepare and process proposal celestiaorg/celestia-app
2 participants
@cmwaters @evan-forbes