vechain / thor

A general purpose blockchain highly compatible with Ethereum's ecosystem
GNU Lesser General Public License v3.0
791 stars 247 forks source link

Adding api-logs-enabled flag #734

Closed otherview closed 1 month ago

otherview commented 1 month ago

Description

Part of the Replay feature functionality - Ticket https://github.com/vechain/protocol-board-repo/issues/183 This flag adds the ability to log incoming requests. Logs are also displayed if the log level is set to debug. It defaults to NOT ENABLED.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

How Has This Been Tested?

Added unit tests.

Checklist:

darrenvechain commented 1 month ago

@otherview does this persist all requests indefinitely ? Maybe we should have a daily cleanup action that clears log files older than X time? Or once it's using more the Y GB of storage

codecov-commenter commented 1 month ago

Codecov Report

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

Project coverage is 61.78%. Comparing base (aedfc6b) to head (b1434a0).

Files Patch % Lines
cmd/thor/main.go 0.00% 8 Missing :warning:
api/api.go 0.00% 3 Missing :warning:
api/request_logger.go 83.33% 2 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #734 +/- ## ========================================== + Coverage 61.77% 61.78% +0.01% ========================================== Files 195 196 +1 Lines 18246 18272 +26 ========================================== + Hits 11271 11290 +19 - Misses 5876 5880 +4 - Partials 1099 1102 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

otherview commented 1 month ago

@otherview does this persist all requests indefinitely ? Maybe we should have a daily cleanup action that clears log files older than X time? Or once it's using more the Y GB of storage

It rotates files, a max of 10 files of 10mbs each. I'll add this to the comments 👍

darrenvechain commented 1 month ago

@otherview does this persist all requests indefinitely ? Maybe we should have a daily cleanup action that clears log files older than X time? Or once it's using more the Y GB of storage

It rotates files, a max of 10 files of 10mbs each. I'll add this to the comments 👍

Should we make it configurable with defaults? I feel like this would be used very quickly on @kgapos mainnet nodes

otherview commented 1 month ago

@otherview does this persist all requests indefinitely ? Maybe we should have a daily cleanup action that clears log files older than X time? Or once it's using more the Y GB of storage

It rotates files, a max of 10 files of 10mbs each. I'll add this to the comments 👍

Should we make it configurable with defaults? I feel like this would be used very quickly on @kgapos mainnet nodes

Yeap, done 👍

darrenvechain commented 1 month ago

@otherview IMO we probably don't need to log requests to the console, as it would make debugging quite difficult in nodes with a lot a traffic

Never mind, my mistake, this is just solo I think?

image
libotony commented 1 month ago

I would prefer the feature to be done outside of the protocol client. Such as reverse proxy with log enabled.

otherview commented 1 month ago

Feature scope change, the api request logs are now issued to the log stdou following the same structure as other logs in the app.