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.

Sign up for GitHub

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

Jump to bottom

Improve info in case of tracing activities #1817

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+11 −2
Open

Improve info in case of tracing activities #1817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/server/src/koaJsonRpc/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,8 @@ export default class KoaJsonRpc {
return;
}

this.logger.trace(`[Request ID: ${this.getRequestId()}] Request body ${JSON.stringify(body)}`);
Copy link
Collaborator

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

Copy link
Collaborator

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?

Copy link
Contributor Author

@Neurone Neurone Aug 27, 2024 •
edited
Loading

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.


ctx.state.methodName = body.method;
const methodName = body.method;

Expand Down Expand Up @@ -192,6 +194,7 @@ export default class KoaJsonRpc {
break;
}
}
this.logger.trace(`[Request ID: ${this.getRequestId()}] Response body ${JSON.stringify(ctx.body)}`);
};
}

Expand Down
8 changes: 7 additions & 1 deletion packages/server/src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import { formatRequestIdMessage } from './formatters';

const mainLogger = pino({
name: 'hedera-json-rpc-relay',
level: process.env.LOG_LEVEL || 'trace',
level: process.env.LOG_LEVEL || 'info',
transport: {
target: 'pino-pretty',
options: {
Expand Down Expand Up @@ -82,6 +82,12 @@ app.getKoaApp().use(async (ctx, next) => {
ctx.state.status
} ${ms} ms`,
);
// log the response
logger.trace(
`${formatRequestIdMessage(ctx.state.reqId)} [${ctx.method}]: ${ctx.state.methodName} ${
ctx.state.status
} ${JSON.stringify(ctx.body)}`,
);
methodResponseHistogram.labels(ctx.state.methodName, `${ctx.status}`).observe(ms);
}
});
Expand Down
2 changes: 1 addition & 1 deletion packages/ws-server/src/webSocketServer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import constants from '@hashgraph/json-rpc-relay/dist/lib/constants';

const mainLogger = pino({
name: 'hedera-json-rpc-relay',
level: process.env.LOG_LEVEL || 'trace',
level: process.env.LOG_LEVEL || 'info',
transport: {
target: 'pino-pretty',
options: {
Expand Down
Loading