Server logger #77

bronzehedwick · 2021-05-02T00:10:38Z

Add eslint config to the root to server both server and browser code bases, instead of duplicating packages.

Add winston for logging with a slack transport. Only invoke the slack logger for warn levels and in production.

Currently the logger is configured only generally; @amcguiga can we collaborate on where the logging needs to take place for the cron task? I took a look over the files but would like to have your input before I blow something up :)

fixes #2

Install eslint in the root for use in both the server and the browser code. Use the airbnb style guide. Add npm commands to lint each code base, and a global npm command to run both from the root.

amcguiga

I'm envisioning it as a daily success or failure log with any error payload based on the cron job. Since that invokes, populateFoia that seems like a good place to invoke it.

50-a-foil-status/server/src/datastore/scripts/populateFoia/index.js

Line 7 in 7984788

const populateFoia = async () => {

IMO all other logs should continue to go to console as before. We just want to get a notification if something goes wrong so we know to dig deeper.

amcguiga · 2021-05-05T00:09:55Z

server/src/app.js


 const app = express();
 app.set("query parser", "simple");

-process.env.NODE_ENV !== "test" && app.use(logger("dev"));
+if (process.env.NODE_ENV !== "test") {


Should the else on this use app.use(morgan("dev"))

I'm not sure I understand; are you saying we should add an else condition here?

Yeah, I'm not clear on what the behavior would be if the environment is test, but I would assume that we would still want to pipe the logs out to console in that circumstance.

Isn't the original logic saying here that we don't want to log to the console when env is test? I also did investigate more; the npm test command sets the test env var (test fail, btw).

With proposed else condition.

> node-server@0.0.0 test > NODE_ENV=test mocha 'src/**/*.test.js' --inline-diffs --exit index routes GET /v1/latest /Users/chris/Sites/50-a-foil-status/server/src/datastore/data GET /v1/latest 200 47.448 ms - 857159 ✓ the response should be an object (70ms) /Users/chris/Sites/50-a-foil-status/server/src/datastore/data GET /v1/latest 200 27.224 ms - 857159 ✓ the response should have a meta property /Users/chris/Sites/50-a-foil-status/server/src/datastore/data ✓ the response should have a foiaList property GET /v1/latest 200 18.480 ms - 857159 /Users/chris/Sites/50-a-foil-status/server/src/datastore/data 1) the response should have a foiaList property with 424 requests GET /v1/latest 200 15.059 ms - 857159 3 passing (196ms) 1 failing 1) index routes GET /v1/latest the response should have a foiaList property with 424 requests: AssertionError: expected [ Array(421) ] to have a length of 424 but got 421 actual expected 421424 at Context.<anonymous> (src/routes/v1/index/routes.test.js:33:41) at processTicksAndRejections (node:internal/process/task_queues:96:5)

Without proposed else condition.

> node-server@0.0.0 test > NODE_ENV=test mocha 'src/**/*.test.js' --inline-diffs --exit index routes GET /v1/latest /Users/chris/Sites/50-a-foil-status/server/src/datastore/data ✓ the response should be an object (64ms) /Users/chris/Sites/50-a-foil-status/server/src/datastore/data ✓ the response should have a meta property /Users/chris/Sites/50-a-foil-status/server/src/datastore/data ✓ the response should have a foiaList property /Users/chris/Sites/50-a-foil-status/server/src/datastore/data 1) the response should have a foiaList property with 424 requests 3 passing (187ms) 1 failing 1) index routes GET /v1/latest the response should have a foiaList property with 424 requests: AssertionError: expected [ Array(421) ] to have a length of 424 but got 421 actual expected 421424 at Context.<anonymous> (src/routes/v1/index/routes.test.js:33:41) at processTicksAndRejections (node:internal/process/task_queues:96:5)

And because I had trouble telling the difference, here's the diff:

9,10c9 < GET /v1/latest 200 47.448 ms - 857159 < ✓ the response should be an object (70ms) --- > ✓ the response should be an object (64ms) 12d10 < GET /v1/latest 200 27.224 ms - 857159 16d13 < GET /v1/latest 200 18.480 ms - 857159 19d15 < GET /v1/latest 200 15.059 ms - 857159 22c18 < 3 passing (196ms) --- > 3 passing (187ms)

amcguiga · 2021-05-05T00:10:30Z

server/src/app.js


 const app = express();
 app.set("query parser", "simple");

-process.env.NODE_ENV !== "test" && app.use(logger("dev"));
+if (process.env.NODE_ENV !== "test") {
+  app.use(morgan("dev", { stream: logger.stream }));


Shouldn't this be production rather than dev? Or am I misunderstanding it?

dev here is the logging level, one below info. I did think it odd that the NODE_ENV was checking "test" instead of "production", but decided to leave it as it is until I fully understood why it was set that way.

Duh, of course. I updated the code to use the logger instead of console.{log,error} in the listed file.

Ah, I see, because all of the other messages are explicitly console.log they would bypass the winston transport functionality? Therefore, only the logger reference would be eligible for winston transports.

Yeah, exactly.

server/src/logger.js

Only invoke the slack logger for error levels and in production. Utilize the logger for placeholder `console.{log,error}`s in the datastore initialization script.

bronzehedwick · 2021-05-06T00:09:48Z

I'm envisioning it as a daily success or failure log with any error payload based on the cron job. Since that invokes, populateFoia that seems like a good place to invoke it.

50-a-foil-status/server/src/datastore/scripts/populateFoia/index.js

Line 7 in 7984788

const populateFoia = async () => {

IMO all other logs should continue to go to console as before. We just want to get a notification if something goes wrong so we know to dig deeper.

Duh, of course. I updated the code to use the logger instead of console.{log,error} in the listed file.

amcguiga · 2021-05-05T00:27:28Z

server/src/logger.js

+  }),
+};
+
+const logger = winston.createLogger({


Should we create another logger reference for messages at the error level? Then we wouldn't have to worry about attaching and detaching the transport for console as all log messages would go to console.

Not sure if we're capable of instantiating multiple loggers to consume in morgan though, still trying to figure that bit out.

Yeah that's a good thought. I'll investigate.

Add eslint config

60f7aa8

Install eslint in the root for use in both the server and the browser code. Use the airbnb style guide. Add npm commands to lint each code base, and a global npm command to run both from the root.

bronzehedwick requested a review from amcguiga May 2, 2021 00:10

amcguiga reviewed May 5, 2021

View reviewed changes

Add slack logger

c064b87

Only invoke the slack logger for error levels and in production. Utilize the logger for placeholder `console.{log,error}`s in the datastore initialization script.

bronzehedwick force-pushed the server-logger branch from 0fdcc3b to c064b87 Compare May 5, 2021 23:59

amcguiga reviewed May 10, 2021

View reviewed changes

bronzehedwick requested review from amcguiga and ashryanbeats August 5, 2021 19:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Server logger #77

Server logger #77

bronzehedwick commented May 2, 2021

amcguiga left a comment

amcguiga May 5, 2021

bronzehedwick May 6, 2021

amcguiga May 10, 2021

bronzehedwick May 13, 2021

amcguiga May 5, 2021

bronzehedwick May 5, 2021

amcguiga May 10, 2021

bronzehedwick May 12, 2021

bronzehedwick commented May 6, 2021

amcguiga May 5, 2021

bronzehedwick May 12, 2021

Server logger #77

Are you sure you want to change the base?

Server logger #77

Conversation

bronzehedwick commented May 2, 2021

amcguiga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bronzehedwick commented May 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment