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

[FVM] Add normalised time per computation logs to transaction execution #5385

Merged
merged 6 commits into from
Feb 26, 2024

Conversation

janezpodhostnik
Copy link
Contributor

@janezpodhostnik janezpodhostnik commented Feb 13, 2024

The normalised time per computation metrics that were added are proving to be useful (https://flowfoundation.grafana.net/goto/a_9XFKTSg?orgId=1), but it would be great to have these logs, so it would be easier to chase down outliers.

@janezpodhostnik janezpodhostnik requested a review from a team February 13, 2024 14:29
@janezpodhostnik janezpodhostnik self-assigned this Feb 13, 2024
engine/execution/computation/computer/result_collector.go Outdated Show resolved Hide resolved
// log the normalized time per computation
// if the computation estimation is correct the value should be 1.
// if the value is greater than 1, the computation estimation is too low; we are underestimating transaction complexity (and thus undercharging).
// if the value is less than 1, the computation estimation is too high; we are overestimating transaction complexity (and thus overcharging).
Copy link
Member

Choose a reason for hiding this comment

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

Does it matter if the transaction failed? Which might have a much smaller time per computation?

Meaning we might not be overcharging, but simply because the transaction failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should be charging correctly even if the transaction fails. The tx still takes time before failing.

@codecov-commenter
Copy link

codecov-commenter commented Feb 15, 2024

Codecov Report

Attention: Patch coverage is 12.50000% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 56.03%. Comparing base (8f4ecc4) to head (59a3b11).

Files Patch % Lines
model/flow/constants.go 0.00% 5 Missing ⚠️
module/metrics/execution.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5385      +/-   ##
==========================================
+ Coverage   55.98%   56.03%   +0.04%     
==========================================
  Files        1026     1018       -8     
  Lines       99902    99473     -429     
==========================================
- Hits        55930    55737     -193     
+ Misses      39677    39446     -231     
+ Partials     4295     4290       -5     
Flag Coverage Δ
unittests 56.03% <12.50%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bluesign
Copy link
Contributor

are there some example outliers?

@janezpodhostnik janezpodhostnik added this pull request to the merge queue Feb 26, 2024
Merged via the queue into master with commit f64ed9e Feb 26, 2024
50 of 51 checks passed
@janezpodhostnik janezpodhostnik deleted the janez/time-per-computation-estimation-logs branch February 26, 2024 14:28
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.

6 participants