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

Bunyan log format support #24

Merged
merged 16 commits into from Mar 20, 2017

Conversation

Projects
None yet
2 participants
@artmnv
Copy link
Contributor

commented Mar 16, 2017

Hello! This is my first open-source pull request. So please give me some feedback. It is very important to me. Thanks.

Should I test /reporters/reporter.js file? Now I just switch off coverage on this file like was on server.js. If yes, can you please help me with idea how to test it.

Issue #22

@artmnv artmnv referenced this pull request Mar 16, 2017

Closed

Add Bunyan log format #22

@artmnv

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2017

There are errors in 2 of 3 travis builds. If I am understanding correctly, it's because of yarn install problem. Please tell me if it is something wrong with my code or I can help you to fix this.

UPD: Same PR was successfully builded by travis in my own repository. artmnv#1
https://travis-ci.org/artmnv/logux-server/builds/211760191
I think it was internal travis error and now we should just rerun build.

@ai

This comment has been minimized.

Copy link
Member

commented Mar 16, 2017

Yeap, I just restart build and they finished goods. In next hour I will review the code.

@ai

This comment has been minimized.

Copy link
Member

commented Mar 16, 2017

Pull request looks very good. Great work.

  1. I think we could move /reporters/reporter.js to server.js. Anyway it is too small.
  2. How developer will change log from text to Bunyan? Seems like docs changes are missed. What is bunyanLogger?
  3. We need one integration test in test/server.js.
@ai

This comment has been minimized.

Copy link
Member

commented Mar 16, 2017

I will put every contributors photos on slide in React London talk. So maybe you want to set avatar in GitHub? :)

@ai

This comment has been minimized.

Copy link
Member

commented Mar 16, 2017

Also what is your name? I will mark task as solved on Cult of Martians, because you did 90% work.

(VK and Twitter accounts will be very helpful later for announce)

@artmnv

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2017

@ai Thank you for your appreciation. :)

  1. Will do.
  2. I think user wants to set logger settings by himself. So I gave him this opportunity by making an bunyanLogger option in logux server constructor.
const logger = bunyan.createLogger({ name: 'logux-server' })
const app = new Server({
  subprotocol: '1.0.0',
  supports: '1.x',
  root: __dirname,
  reporter: 'bunyan',
  bunyanLogger: logger
})

What do you think?
3. I'll try. :)

I'll set avatar photo.
Links: https://vk.com/index.ascx https://twitter.com/aartmnv

@ai

This comment has been minimized.

Copy link
Member

commented Mar 16, 2017

I think that developers will use text log in development and Bunyan in production. In this case changing logger in code become too tricky. We tried to have different dev/prod options on Amplifr, but it looks ugly.

Alaso DevOps like “The Twelve-Factor” philosophy.

So I think argument and environment variable will be more useful:

LOGUX_LOGGER=bunyan npm start

In this case DevOps will be able to change logger without asking JS developer to help.

What do you think about this UX?

@artmnv

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2017

@ai This was my first thought. And I implemented it this way. But than I dive deeper into the bunyan and realise that there are a bunch of setting. I don't use bunyan. So maybe I don't understand how it works. But I didn't find any system-wide settings. Only for one project with it's code. This is initialisation from official readme:

var bunyan = require('bunyan');
var log = bunyan.createLogger({
    name: <string>,                     // Required
    level: <level name or number>,      // Optional, see "Levels" section
    stream: <node.js stream>,           // Optional, see "Streams" section
    streams: [<bunyan streams>, ...],   // Optional, see "Streams" section
    serializers: <serializers mapping>, // Optional, see "Serializers" section
    src: <boolean>,                     // Optional, see "src" section

    // Any other fields are added to all log records as is.
    foo: 'bar',
    ...
});

As a user of this logger I'll want to set at least name and streams setting. We can proxy them but I think it's ugly. We also can't set same options to all projects. There will be no opportunity to differ logs from each other. That's why I made this decision.

Maybe I don't understand something? :)

@ai

This comment has been minimized.

Copy link
Member

commented Mar 16, 2017

@artmnv yeap, it is good question :D. Maybe this questions could help us:

  1. Does this options is really useful for most of users? We could provide argument/env for default Bunyan settings. And allow to set Bunyan instance manually for users, who really need it.
  2. What UX is used in other applications with Bunyan logger?
@artmnv

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2017

@ai I think it will be great if we'll make name and file (stderr or filename) settings available from env with zero code. And will provide logux-server's option for 5-10% of people who really need all power of bunyan streams customisation. Deal? :D

@ai

This comment has been minimized.

Copy link
Member

commented Mar 16, 2017

@artmnv do we need to specify files? I thought that in Bunyan you just write logs to stdout/stderr. But maybe I am wrong?

Sure, I like idea with bunyan (we could make options shorter with logger=text/bunyan and bunyan names) for special cases.

@ai

This comment has been minimized.

Copy link
Member

commented Mar 17, 2017

I have a better idea, instead of having reportProcess function in server.js. Right now you created a wrap reportProcess function. This funciton on every call check options and execute other function. But options are persistent.

You could do it better. Just check options once in Server constructor and pick different reporter for BaseServer:

let reporter
if ( weNeedJSONLog ) {
  reporter = jsonReporter
} else {
  reporter = textReporter
}
super(…, reporter)
@artmnv

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2017

Thank you for your ideas!

To make env/cli params we should provide them to server constructor somehow. If you remember, now env/cli params are loading by Server.loadOptions method. But they are passing to Server.listen and not to constructor.

const app = new Server({
  subprotocol: '1.0.0',
  supports: '1.x',
  root: __dirname,
  reporter: 'bunyan'
})

app.listen(app.loadOptions(process, {}))

Brainstorming:

  1. Add loadServerOptions and call it Server(loadServerOptions(...)). Initialisation is too complex.
  2. Add reporter option to loadOptions and call it on Server(loadOptions(...)) too. Listen options in server options can distract user and can lead to bugs.
  3. Leave it. We can't change subprotocol, root, nodeId, timeout, ping etc settings by env/cli params. Maybe we should leave this for another task.

I like the 3rd option. xD But what do you think? Maybe you see a better solution?

@ai

This comment has been minimized.

Copy link
Member

commented Mar 18, 2017

I think we could move all listen arguments to constructor. Anyway is very weird, that we have 2 settings. Also tests are very complicated right now, because we need to start server just to check default port :).

@artmnv

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2017

@ai Doesn't anybody want to listen to several ports? Does it work this way now? If no, than it's better to merge configs. And maybe even auto-listen after server creation. I think it's not good for UX to have server in untuned state. Especially if there is no another useful scenario for it initialisation.

@ai

This comment has been minimized.

Copy link
Member

commented Mar 18, 2017

Totally agree about merging configs.

Auto-listen has some problem:

  1. Test will be much faster if we will not listen port while testing configs.
  2. Listening a port could not be started immediately. In many cases user may need a Promise, when we server was really started.
  3. All node servers API has separated constructor and listen method.
@artmnv

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2017

We can bypass first two problems. But if it's common style for node, it will be better to follow it. Okay. Anything else I can help you with this PR?

@ai

This comment has been minimized.

Copy link
Member

commented Mar 18, 2017

@artmnv do you want to merge options? I could do it by my own too, anyway you already did most of work.

@artmnv

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2017

@ai I'd love to. But don't you think it's better to do in separate PR? This is solid PR which does what the title says.

@ai

This comment has been minimized.

Copy link
Member

commented Mar 18, 2017

Sure. I will accept it today and ping you when project will be ready for second PR :).

@artmnv

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2017

@ai Thank you! Feel free to ask any help related to this PR. :)

@ai ai merged commit 0cea3d8 into logux:master Mar 20, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.