Skip to content

Conversation

mstriemer
Copy link
Contributor

Server logging with bunyan, we don't currently have any client side logging other than printing an error if redux state fails to load.

Fixes mozilla/addons#9547.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4728d5e on mstriemer:logging-182 into 52a52eb on mozilla:master.

package.json Outdated
"start:disco": "better-npm-run start:disco",
"start:search": "better-npm-run start:search",
"start:disco": "npm run version-check && NODE_PATH='./:./src' NODE_APP_INSTANCE=disco node bin/server.js",
"start:search": "npm run version-check && NODE_PATH='./:./src' NODE_APP_INSTANCE=search node bin/server.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, i was begginning to get to the point where I was thinking we should kill all the app specific start up stuff in favour of just using NODE_APP_INSTANCE (aside from maybe the dev things for convenience).

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 refactored this to have a start again so once the deployments are updated to use NODE_APP_INSTANCE=disco npm start we can remove the old start:disco commands.

@muffinresearch
Copy link
Contributor

If we get to a point of having log code in shared code presumably bunyan could do the right thing contextually?

global.CLIENT = false;
global.SERVER = true;
global.DEVELOPMENT = env === 'development';
global.DEVELOPMENT = isDevelopment;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can kill these and the definePlugin versions of the same in the webpack conf since the config exposes what we need. Also I'm not sure if we need the CLIENT and SERVER ones? Maybe those are still useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They don't seem to be read anywhere, I've removed them.

@muffinresearch
Copy link
Contributor

r+wc looks good.

@mstriemer
Copy link
Contributor Author

I'm not sure it's designed to work in the browser but I can look into it. What would the right thing be? Logging to console or sending them to a server?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 90a97af on mstriemer:logging-182 into 52a52eb on mozilla:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 37812a1 on mstriemer:logging-182 into 52a52eb on mozilla:master.

@mstriemer
Copy link
Contributor Author

So I tried including bunyan in a client view and webpack blows up because it can't find fs. So it looks like bunyan will not help for any client side logging.

@mstriemer mstriemer merged commit 250f844 into mozilla:master May 5, 2016
@mstriemer mstriemer deleted the logging-182 branch May 5, 2016 20:04
@muffinresearch
Copy link
Contributor

muffinresearch commented May 5, 2016

It should work see trentm/node-bunyan#282 you might need to do this though trentm/node-bunyan#282 (comment)

@muffinresearch
Copy link
Contributor

We'll want just any browser logging to point to the relevant console log func. It'll probably make sense to minimize the format a bit so there's not too much data displayed.

@muffinresearch
Copy link
Contributor

Can deal with it later on though since this is ready to land.

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.

Log all the things
3 participants