Skip to content

Logs support. #243

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

Merged
merged 1 commit into from
Feb 12, 2016
Merged

Logs support. #243

merged 1 commit into from
Feb 12, 2016

Conversation

peterdotjs
Copy link
Contributor

Added /logs endpoint with basic logger and LoggerAdapter.

  • Default logger is based on Winston with a custom query method.
  • Only GET for /logs is implemented. Query params designed to match loggly interface.
  • Log files are created each day. INFO and ERROR leveled logs are supported.
  • LoggerAdapter requires info, error and query methods to be implemented.

@peterdotjs
Copy link
Contributor Author

TODO: Add unit tests prior to merge.

@gfosco gfosco added the accepted label Feb 4, 2016
@gfosco gfosco assigned peterdotjs and unassigned gfosco Feb 4, 2016
@peterdotjs peterdotjs mentioned this pull request Feb 4, 2016
@flovilmart
Copy link
Contributor

@peterdotjs Would it be possible to syndicate those logs in a mongo? So multiple server would write in the logs DB and we could keep the order in the case of multiple front-end instances?

@peterdotjs
Copy link
Contributor Author

@flovilmart Absolutely! Either the community or team should work on creating an adapter to support the multiserver case.

@flovilmart
Copy link
Contributor

@peterdotjs so in that case, could you refactor a bit the logger as FileLogger.js? This way we'll be able to drop in more logging solution.

@peterdotjs
Copy link
Contributor Author

@flovilmart yup that sounds good I'll rename to match the transport.

@flovilmart
Copy link
Contributor

Wonderful! In the end that's just Winston transports to add.

@nlutsenko nlutsenko removed the accepted label Feb 9, 2016
Added /logs endpoint with basic logger and LoggerAdapter.
@peterdotjs
Copy link
Contributor Author

Updated with test, es6 and new adapter/controller style.

@facebook-github-bot
Copy link

@peterdotjs updated the pull request.

const MILLISECONDS_IN_A_DAY = 24 * 60 * 60 * 1000;

// only allow request with master key
let enforceSecurity = (auth) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nikita made a middleware for enforcing master key that you should probably use instead. Or if you are too busy, we can update this later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll defer this for now and add it in when I add the docs.

@drew-gross
Copy link
Contributor

If you could either make this experimental (using a env var or something) or add unit or e2e tests I would appreciate it :) up to you though, if you think the code is solid, just ship it. It's not like it will corrupt any DBs or anything.

@peterdotjs
Copy link
Contributor Author

@drew-gross FileLoggerAdapter.spec is basically testing e2e in creating the log and querying it. I'll move forward with the merge.

peterdotjs added a commit that referenced this pull request Feb 12, 2016
@peterdotjs peterdotjs merged commit 23c1284 into master Feb 12, 2016
@peterdotjs peterdotjs deleted the peterjs.logs branch February 12, 2016 22:45
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.

6 participants