[API] [PFNs] improve observability of view function and simulate usage #11696
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #11696 +/- ##
=======================================
Coverage 70.9% 70.9%
=======================================
Files 791 791
Lines 180312 180406 +94
=======================================
+ Hits 127990 128067 +77
- Misses 52322 52339 +17 ☔ View full report in Codecov by Sentry. |
bchocho
force-pushed
the
brian/api-better-observability
branch
from
89ce918 to
9102ca2
Compare
bchocho
changed the title
bchocho
requested review from
JoshLind,
gregnazario,
davidiw,
wrwg,
zekun000,
vgao1996,
georgemitenkov,
banool and
0xmaayan
as code owners
gregnazario
reviewed
Jan 19, 2024
aptos-move/aptos-vm/src/aptos_vm.rs
Outdated
| ) -> Result<Vec<Vec<u8>>> { | ||
| ) -> Result<(Vec<Vec<u8>>, u64)> { |
There was a problem hiding this comment.
Personally, it would be preferred to be a struct, so the outputs are known names. That way it won't require someone to read the code at the end of the function to determine it's the gas used.
There was a problem hiding this comment.
+1, we can have something similar to TransactionOutput or VMOutput structs
api/src/transactions.rs
Outdated
| entry_function.module(), | ||
| &entry_function.function().into(), | ||
| ), | ||
| TransactionPayload::Multisig(_) => "MultiSig".to_string(), |
There was a problem hiding this comment.
MultiSig has an entry function tied to it, so I would probably add that here as well. It's in the inner type.
api/src/transactions.rs
Outdated
| @@ -1220,6 +1220,19 @@ impl TransactionsApi { | |||
| _ => ExecutionStatus::MiscellaneousError(None), | |||
| }; | |||
|
|
|||
| let stats_key = match txn.payload() { | |||
| TransactionPayload::Script(_) => "Script".to_string(), | |||
There was a problem hiding this comment.
You could add the script hash here
bchocho
force-pushed
the
brian/api-better-observability
branch
from
002ce11 to
20eab1c
Compare
|
@gregnazario @georgemitenkov thanks! Ready for a new review -- I've added a proper struct, also making changes so view functions that fail will still get gas usage logged. |
grao1991
approved these changes
Jan 23, 2024
JoshLind
approved these changes
Jan 25, 2024
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
… can have used gas
…peration's gas spikes (and then can dig into logs which function to attribute to)
bchocho
force-pushed
the
brian/api-better-observability
branch
from
20eab1c to
0758558
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
bchocho
added a commit
that referenced
this pull request
Jan 26, 2024
#11696) ### Description On PFNs, periodically (every minute) log the top-9 gas-using view functions and simulate transactions. This gives observability to the PFNs usage. Utilizes a small cache that is reset every minute. Alternatively, such high-cardinality data would not work with prometheus. Requires minimal changes to propagate gas usage. ### Test Plan Deploy PFN locally, observe log messages.
bchocho
added a commit
that referenced
this pull request
Jan 30, 2024
#11696) ### Description On PFNs, periodically (every minute) log the top-9 gas-using view functions and simulate transactions. This gives observability to the PFNs usage. Utilizes a small cache that is reset every minute. Alternatively, such high-cardinality data would not work with prometheus. Requires minimal changes to propagate gas usage. ### Test Plan Deploy PFN locally, observe log messages.
bchocho
added a commit
that referenced
this pull request
Jan 30, 2024
#11696) (#11791) ### Description On PFNs, periodically (every minute) log the top-9 gas-using view functions and simulate transactions. This gives observability to the PFNs usage. Utilizes a small cache that is reset every minute. Alternatively, such high-cardinality data would not work with prometheus. Requires minimal changes to propagate gas usage. ### Test Plan Deploy PFN locally, observe log messages.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
On PFNs, periodically (every minute) log the top-9 gas-using view functions and simulate transactions. This gives observability to the PFNs usage.
Utilizes a small cache that is reset every minute. Alternatively, such high-cardinality data would not work with prometheus.
Requires minimal changes to propagate gas usage.
Test Plan
Deploy PFN locally, observe log messages.