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

[Access] Reduce logging for script executions #5959

Merged
merged 3 commits into from
Jul 5, 2024

Conversation

peterargue
Copy link
Contributor

@peterargue peterargue commented May 21, 2024

This PR reduces logging related to script execution on Access nodes by making the following changes:

  • Log script results comparison differences at Debug level
  • Use the logging frequency gate for all log messages that contain the script. When the script has been logged recently, just log the hash
  • Log script results in base64 instead of hex.

@codecov-commenter
Copy link

codecov-commenter commented May 21, 2024

Codecov Report

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

Project coverage is 55.83%. Comparing base (55fd3bb) to head (087b11e).

Files Patch % Lines
engine/access/rpc/backend/backend_scripts.go 31.81% 12 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5959      +/-   ##
==========================================
+ Coverage   55.81%   55.83%   +0.01%     
==========================================
  Files        1129     1129              
  Lines       89205    89210       +5     
==========================================
+ Hits        49794    49808      +14     
+ Misses      34656    34643      -13     
- Partials     4755     4759       +4     
Flag Coverage Δ
unittests 55.83% <53.12%> (+0.01%) ⬆️

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.

Comment on lines 250 to 252
if err != nil {
return err
}
Copy link
Member

Choose a reason for hiding this comment

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

Not a part of your diff, but we should move this up to immediately after the error is returned, on line 246.

@@ -318,6 +320,9 @@ func isInvalidArgumentError(scriptExecutionErr error) bool {

// shouldLogScript checks if the script hash is unique in the time window
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// shouldLogScript checks if the script hash is unique in the time window
// shouldLogScript checks whether we should log a script, based on whether we have seen it recently
// and the current log level.

@peterargue peterargue added this pull request to the merge queue Jul 5, 2024
Merged via the queue into master with commit ab87266 Jul 5, 2024
55 checks passed
@peterargue peterargue deleted the petera/less-logging-script-exec branch July 5, 2024 01:57
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.

4 participants