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

feat: SCSC metrics for #TXs that fail pureCheck #16077

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

david-bakin-sl
Copy link
Member

Description:

SCSC metrics for #TXs that fail pureCheck:

  • counts for CONTRACT_CREATE / CONTRACT_CALL / ETHEREUM_TRANSACTION that fail pureCheck for any reason
  • and for those that fail pureCheck because they don't even have the intrinsic gas amount
  • and for Type 3 format (BLOB) Ethereum transactions (that we don't support)

Related issue(s):

Fixes #16076

Notes for reviewer:

  • Key class is ContractMetrics to encapsulate all SCSC metrics; and currently it is used in ContractCreateHandler, ContractCallHandler, and EthereumTransactionHandler
  • Added a Supplier<Metrics> to the AppContext so smart contract service (and others) can get the metrics instance from the app.
    • There are quite a few unit tests that needed to be modified for this change as they create an AppContext out of thin air
    • This may not be necessary. I was unable to inject a Metrics object where needed but apparently token service can in CryptoGetAccountBalanceHandler - I need to find out how
    • Plus it is somewhat awkward as the AppContext is created before the Metrics instance is gotten from the platform
    • And this is why I have a locked "once only" method to initialize ContractMetrics - I'll be asking people about how to do that better as well

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@david-bakin-sl david-bakin-sl added the Hedera Smart Contract Service Issues related to the Hedera Smart Contract Service. label Oct 22, 2024
@david-bakin-sl david-bakin-sl added this to the v0.56 milestone Oct 22, 2024
@david-bakin-sl david-bakin-sl self-assigned this Oct 22, 2024
@david-bakin-sl david-bakin-sl force-pushed the 16076-scsc-metrics-purecheck branch from 8811c9d to b8abf1c Compare October 22, 2024 08:12
andrewb1269hg
andrewb1269hg previously approved these changes Oct 22, 2024
Copy link
Contributor

@andrewb1269hg andrewb1269hg left a comment

Choose a reason for hiding this comment

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

Reviewed the following file:
hedera-node/hedera-smart-contract-service-impl/src/main/java/module-info.java

andrewb1269hg
andrewb1269hg previously approved these changes Oct 23, 2024
Copy link
Contributor

@andrewb1269hg andrewb1269hg left a comment

Choose a reason for hiding this comment

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

Reviewed the following files:
hedera-node/hedera-app-spi/src/main/java/module-info.java
hedera-node/hedera-smart-contract-service-impl/src/main/java/module-info.java

These two files look good.

@david-bakin-sl
Copy link
Member Author

Switching to draft until (extensive) rebase is confirmed.

Copy link

codacy-production bot commented Nov 15, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.01% (target: -1.00%) 84.21%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (98c37b9) 97773 63984 65.44%
Head commit (6009a60) 97882 (+109) 64065 (+81) 65.45% (+0.01%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#16077) 152 128 84.21%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Copy link

codecov bot commented Nov 15, 2024

Codecov Report

Attention: Patch coverage is 80.92105% with 29 lines in your changes missing coverage. Please review.

Project coverage is 63.74%. Comparing base (c0dfdfc) to head (6009a60).
Report is 4 commits behind head on develop.

Files with missing lines Patch % Lines
...ce/contract/impl/exec/metrics/ContractMetrics.java 82.75% 14 Missing and 1 partial ⚠️
...ract/impl/handlers/EthereumTransactionHandler.java 62.50% 3 Missing and 3 partials ⚠️
...app/service/contract/impl/ContractServiceImpl.java 50.00% 3 Missing ⚠️
...-app/src/main/java/com/hedera/node/app/Hedera.java 0.00% 2 Missing ⚠️
...l/handlers/AbstractContractTransactionHandler.java 88.88% 1 Missing and 1 partial ⚠️
...app/workflows/standalone/TransactionExecutors.java 66.66% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             develop   #16077    +/-   ##
===========================================
  Coverage      63.74%   63.74%            
- Complexity     20493    20511    +18     
===========================================
  Files           2538     2540     +2     
  Lines          95008    95117   +109     
  Branches        9931     9937     +6     
===========================================
+ Hits           60559    60636    +77     
- Misses         30834    30862    +28     
- Partials        3615     3619     +4     
Files with missing lines Coverage Δ
.../main/java/com/hedera/node/app/spi/AppContext.java 33.33% <ø> (ø)
...a/com/hedera/node/app/services/AppContextImpl.java 100.00% <ø> (ø)
...a/com/hedera/node/config/data/ContractsConfig.java 100.00% <ø> (ø)
...ce/contract/impl/handlers/ContractCallHandler.java 100.00% <100.00%> (ø)
.../contract/impl/handlers/ContractCreateHandler.java 100.00% <100.00%> (ø)
...app/workflows/standalone/TransactionExecutors.java 91.07% <66.66%> (-1.53%) ⬇️
...-app/src/main/java/com/hedera/node/app/Hedera.java 56.79% <0.00%> (-0.32%) ⬇️
...l/handlers/AbstractContractTransactionHandler.java 88.88% <88.88%> (ø)
...app/service/contract/impl/ContractServiceImpl.java 85.00% <50.00%> (-15.00%) ⬇️
...ract/impl/handlers/EthereumTransactionHandler.java 77.46% <62.50%> (-3.14%) ⬇️
... and 1 more

... and 4 files with indirect coverage changes

Impacted file tree graph

@david-bakin-sl david-bakin-sl force-pushed the 16076-scsc-metrics-purecheck branch from 193eb33 to 94894bd Compare November 19, 2024 04:34
@david-bakin-sl david-bakin-sl marked this pull request as ready for review November 19, 2024 04:34
andrewb1269hg
andrewb1269hg previously approved these changes Nov 19, 2024
Copy link
Contributor

@andrewb1269hg andrewb1269hg left a comment

Choose a reason for hiding this comment

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

Review applies to:
hedera-node/hedera-smart-contract-service-impl/src/main/java/module-info.java

Approved.

andrewb1269hg
andrewb1269hg previously approved these changes Nov 20, 2024
@david-bakin-sl david-bakin-sl force-pushed the 16076-scsc-metrics-purecheck branch 3 times, most recently from 71af87a to 860cfa7 Compare November 22, 2024 22:08
@david-bakin-sl david-bakin-sl force-pushed the 16076-scsc-metrics-purecheck branch 5 times, most recently from 0650a76 to 5189000 Compare December 3, 2024 00:45
@david-bakin-sl david-bakin-sl force-pushed the 16076-scsc-metrics-purecheck branch from 5189000 to 89f4a1b Compare December 3, 2024 17:40
Copy link
Member

@lukelee-sl lukelee-sl left a comment

Choose a reason for hiding this comment

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

LGTM

- counts for CONTRACT_CREATE / CONTRACT_CALL / ETHEREUM_TRANSACTION that fail pureCheck for any reason
- and for those that fail pureCheck because they don't even have the intrinsic gas amount
- and for Type 3 format (BLOB) Ethereum transactions (that we don't support)

By:
- Change plumbing for metrics into smart contract service
- Metrics are controlled by feature flags in `ContractsConfig`
- Added `AbstractContractTransactionHandler` abstract class to hold a growing
  amount of commonality in smart contract transaction handler classes.

Signed-off-by: David S Bakin <[email protected]>
@david-bakin-sl david-bakin-sl force-pushed the 16076-scsc-metrics-purecheck branch from 89f4a1b to 6009a60 Compare December 3, 2024 20:20
david-bakin-sl added a commit that referenced this pull request Dec 3, 2024
Cherry pick of #16077 (from the PR, which has not yet merged to develop)

Signed-off-by: David S Bakin <[email protected]>
@david-bakin-sl david-bakin-sl merged commit 63f036f into develop Dec 3, 2024
43 of 44 checks passed
@david-bakin-sl david-bakin-sl deleted the 16076-scsc-metrics-purecheck branch December 3, 2024 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hedera Smart Contract Service Issues related to the Hedera Smart Contract Service.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: add smart contract service metrics for txs that fail pureCheck
4 participants