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

Logging Refactoring #976

Merged
merged 5 commits into from
Sep 22, 2022
Merged

Logging Refactoring #976

merged 5 commits into from
Sep 22, 2022

Conversation

israelboudoux
Copy link
Contributor

@israelboudoux israelboudoux commented Sep 21, 2022

The main goal of this PR is to refactoring the logs to accomplish with the requirement of having our logs compliant to the JSON format.

Logs are recommended to be echoed in JSON because it is simple for reading and is easily ingested by Monitoring tools, which also facilitates the filtering by in Logging tools like CloudWatch, Kibana, Splunk (e.g. 'person.name = 'Bob'').

Below is an example of log output in JSON format.

{"level":"debug","time": "2022-09-20T21:24:08.918Z","message":"Saving block","block":"{\n  \"_id\": \"0xa61bfe67ad6e8a8b4dec54049fcaea26aca10285e9f0cf29f97cab9b379bc9a0\",\n  \"blockNumber\": 1543,\n  \"transactionHashL1\": \"0x3bcef69a31d7dc6ce65014a87a1a8c493d835362fd0f59b1bda4b41e86af5120\",\n  \"timeBlockL2\": \"2022-09-20T21:23:51.000Z\",\n  \"proposer\": \"0xfeEDA3882Dd44aeb394caEEf941386E7ed88e0E0\",\n  \"root\": \"0x059b5466795648f3acddddd947a0f905803e666d816bfe211df0eadeedd68545\",\n  \"leafCount\": 18,\n  \"blockNumberL2\": 9,\n  \"previousBlockHash\": \"0xf9af83d91f69872467ff12ca510985414dc3ccabc161a5ef46bfa0ee423e22e0\",\n  \"transactionHashesRoot\": \"0xa298346edcb34c8bf64a5400b4047e608ef85cfecf38ed3d47d2e5107ec4231a\",\n  \"blockHash\": \"0xa61bfe67ad6e8a8b4dec54049fcaea26aca10285e9f0cf29f97cab9b379bc9a0\",\n  \"nCommitments\": 2,\n  \"transactionHashes\": [\n    \"0xdddeb46b51fc4034be062f904872d8c1e80d7a35830dccec376f45851dc511a9\",\n    \"0xdfdeb95eec4d7ae66f2489192861fac7462b8394734858d8249e8223101817e2\"\n  ]\n}"}

Regarding other changes in this PR:

  • Removing commented code;
  • Applying the recommended comment block (/* */) when the comment spans for more than one line;
  • Applying document comment (/** */) where necessary;
  • Some changes were done in some files to improve the readability of the code.

@israelboudoux israelboudoux marked this pull request as ready for review September 21, 2022 20:08
Copy link
Contributor

@signorecello signorecello left a comment

Choose a reason for hiding this comment

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

Just some quick things to check

Copy link
Contributor

@imagobea imagobea left a comment

Choose a reason for hiding this comment

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

Thank you @israelboudoux , LGTM!!

@@ -17,19 +17,16 @@ import { setMakeNow } from '../services/block-assembler.mjs';
const router = express.Router();

router.post('/check', async (req, res, next) => {
logger.debug('block endpoint received POST');
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious about why did u remove (and not replace) some of the logs in this file (?)

Copy link
Contributor Author

@israelboudoux israelboudoux Sep 22, 2022

Choose a reason for hiding this comment

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

There is a pending PR that will add the capability of logging automatically the Request and Response information for all endpoints when the log level DEBUG is enabled. Actually, I wasn't supposed to remove this now, only after merging the mentioned PR, but it isn't a big deal.

@signorecello signorecello merged commit d71bd65 into master Sep 22, 2022
@signorecello signorecello deleted the israelboudoux/logging-refactoring branch September 22, 2022 21:31
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.

3 participants