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

Server logger #77

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bronzehedwick
Copy link
Collaborator

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.
Copy link
Collaborator

@amcguiga amcguiga left a comment

Choose a reason for hiding this comment

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

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.

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.


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

process.env.NODE_ENV !== "test" && app.use(logger("dev"));
if (process.env.NODE_ENV !== "test") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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)


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 }));
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, exactly.

server/src/logger.js Show resolved Hide resolved
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
Copy link
Collaborator Author

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.

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.

}),
};

const logger = winston.createLogger({
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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.

Add a server logger
2 participants