-
Notifications
You must be signed in to change notification settings - Fork 63
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
Improve info in case of tracing activities #1817
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Giuseppe Bertone <giuseppe.bertone@swirldslabs.com>
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1817 +/- ##
==========================================
+ Coverage 77.73% 77.75% +0.02%
==========================================
Files 40 40
Lines 3023 3026 +3
Branches 603 603
==========================================
+ Hits 2350 2353 +3
Misses 489 489
Partials 184 184
☔ View full report in Codecov by Sentry. |
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.
Simple enough change.
@Neurone can you add an example of the logs emitted after a simple call to say eth_getTransactionCount(..)
One concern here is the expansion of logs and this will result in duplication of log details. Logging every request and response body can be very large.
All the important details should be logged down the line. Are there specific HTTP properties you're looking for that are missing?
Hi Nana, those lines show the payload sent by the client and the relay's response, so it depends. Adding those logs was essential, for example, to track down the bug described in #1802, and here is an example of what you can see before and after adding that tracing while receiving requests for that use case: Old log, with
New log, with
The log's type is To avoid bloating the log files, I also suggested changing the default log level from |
@@ -119,6 +119,8 @@ export default class KoaJsonRpc { | |||
return; | |||
} | |||
|
|||
this.logger.trace(`[Request ID: ${this.getRequestId()}] Request body ${JSON.stringify(body)}`); |
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.
This may result in repeated info in the logs as some methods log the contents of the body which are params.
In a few cases that's transaction bytes or a whole contract.
So we might need some coordination on that
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.
@Neurone Is this still an issue?
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.
The code of the PR must be probably updated at this point, but yes, imho the issue this PR tries to solve is still there.
I think when as a developer I set TRACE as a log level, I want to see anything because I'm doing this for heavy debugging activities. The PR includes the answers and the responses bodies to the logs, and this info is essential for debugging purposes.
At the same time, I think the default log level should be INFO, not TRACE as we use now. In general I think using INFO as default is a good practice, and in this case it shows another good reason why. If we use TRACE as our default, when adding more data to the log it seems too much (as reported by Nana) but the problem for me it's not there, because when I enable trace I want to see everything, and duplicated data are not an issue at all and in some cases are very useful (because I can see nothing happened to those data during the business flow).
If this make sense, I can work on updating the PR for being merged, if you disagree or want to create a different broader task for this topic, you can close this PR.
Description:
While conducting debug activities you often enable trace log level, and you want to check the actual requests and response between client and server (i.e., #1802).
This PR:
info
instead oftrace
Related issue(s):
N/A
Notes for reviewer:
N/A
Checklist