-
Notifications
You must be signed in to change notification settings - Fork 75
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
Add a gas/s metric for gas consuming endpoints #939
Add a gas/s metric for gas consuming endpoints #939
Conversation
Signed-off-by: nikolay <[email protected]>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #939 +/- ##
==========================================
- Coverage 74.40% 74.24% -0.16%
==========================================
Files 28 28
Lines 1891 1899 +8
Branches 356 360 +4
==========================================
+ Hits 1407 1410 +3
- Misses 381 386 +5
Partials 103 103
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Please add a description |
Signed-off-by: nikolay <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per other suggestions let's make this clean and have a separate metric.
Preserve the same labels as i believe they are applicable
Signed-off-by: nikolay <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, removing block.
Left some final suggestions.
Please get @AlfredoG87 additional sign off before merging.
Signed-off-by: nikolay <[email protected]>
Signed-off-by: nikolay <[email protected]>
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Thank you for addressing all the comments 💯
Description:
Currently, we capture the HBAR spent on gas-consuming calls but we don't capture the actual gas
Note: This may require a mirror node call to view contract results for "Gas Used"
Note 2: Contract calls do not emit records so it's difficult to capture gas used in that case.
Note 3: gas charged = 0.8 * gas limit. Also gas charged >= gas burnt. Example from Danno. "If I did a 35k gas defi trade, the gas limit at 300k, the gas used will be 240k. We should call this "gas charged" even though the EVM only used 35k, "gas burnt."..."
Related issue(s):
Fixes #876
Notes for reviewer:
Checklist