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

[do not merge] Add ability to generate JSON by passing an Object as the format #123

Closed
wants to merge 1 commit into from

Conversation

bmeck
Copy link

@bmeck bmeck commented Oct 4, 2016

Various logging systems (notably the ELK stack) can use JSON metadata when creating queries and dashboards. This PR adds the ability to pass an Object as an options.format which will then be crawled and template all strings found in that Object.

If this seems reasonable I will add tests and docs, but wanted to pass by and see if this is reasonable without creating a fork.

Example:

var morgan=require('morgan');
var format = {m:':method'};
var logger=morgan(undefined, {format})
logger({method:'aha',headers:{}},{},()=>{});
// {"m":"aha"}

@dougwilson
Copy link
Contributor

As far as I can tell, all the code you added in the PR is 100% achievable in user-land and does not require a fork, right? Because the format option to this module accepts a function.

@bmeck
Copy link
Author

bmeck commented Oct 4, 2016

@dougwilson I guess it could be done in user land. Figured that having logs that are not easily searched as text blobs would be somewhat common. though I guess I would assume documentation would mean that the API for those functions would be stable? I can write some if it is not internal/likely to change.

@dougwilson
Copy link
Contributor

I guess it could be done in user land.

So my comment was really just to make sure I was understanding if I was missing something or not since you said you would need to fork, I was assuming there was something in there that required source code modification to function, but I didn't see anything.

Figured that having logs that are not easily searched as text blobs would be somewhat common.

I'm not 100% sure this comment means :) At least currently, morgan outputs the formats that all the major web log analyzers expect. AWStats is a very popular web log analyzer, for example. AWStats and similar tools work out-of-the-box with the Apache combined format, for example. I have never encountered any tools that are looking for some sort of JSON output, for example, unless you are using some kind of custom solution (and so you'd probably be using the format as a function anyway).

though I guess I would assume documentation would mean that the API for those functions would be stable? I can write some if it is not internal/likely to change.

I'm not sure what that means?

@dougwilson dougwilson self-assigned this Oct 4, 2016
@dougwilson dougwilson added the pr label Oct 4, 2016
@bmeck
Copy link
Author

bmeck commented Oct 4, 2016

I'm piping data directly into Elasticsearch/Logstash and reading it via Kibana's UI dashboards (the ELK stack). Elasticsearch does not parse strings passed to it, and so it just appears as a big text blob which isn't very useful.

though I guess I would assume documentation would mean that the API for those functions would be stable? I can write some if it is not internal/likely to change.

I'm not sure what that means?

The docs mention that you can pass a function, but do not state what the function is/should do. Was just saying if thats the proper case I can document the (tokens,req,res)=>string|null signature and how to use tokens so the use of a function is documented/public knowledge.

@bmeck bmeck closed this Oct 6, 2016
@bmeck bmeck mentioned this pull request Oct 6, 2016
@dougwilson
Copy link
Contributor

The docs mention that you can pass a function, but do not state what the function is/should do. Was just saying if thats the proper case I can document the (tokens,req,res)=>string|null signature and how to use tokens so the use of a function is documented/public knowledge.

Yes, absolutely! :) I just looked over the README and for some reason though it was better documented, but you're right: it just mentions you can give a function, but no real details. A pull request to add documentation around this would be useful indeed 👍

@bmeck
Copy link
Author

bmeck commented Oct 6, 2016

@dougwilson did that with #124 , which does docs and adds some tests explicitly to comply with existing behavior / what docs were added.

@dougwilson
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants