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

Gelf compatibility #5

Merged
merged 6 commits into from
Aug 23, 2019
Merged

Conversation

jfahrenkrug
Copy link
Contributor

This PR introduces breaking changes to pino-gelf, hence the major
version bump.

The GELF spec at http://docs.graylog.org/en/3.0/pages/gelf.html#gelf-payload-specification
specifies a flat JSON structure. Custom fields have to be prefixed with an underscore and can
only contain string values. We do that now. All tests pass.

Also, a passthrough mode is being added. That way we can pass pino messages to pino-gelf and then
pipe pino-gelf's output to another tool. That way we can both send to a graylog server and pretty-print.

This addresses #4

Any feedback is welcome!

This commit introduces breaking changes to pino-gelf, hence the major
version bump.

The GELF spec at http://docs.graylog.org/en/3.0/pages/gelf.html#gelf-payload-specification
specifies a flat JSON structure. Custom fields have to be prefixed with an underscore and can
only contain string values. We do that now. All tests pass.

Also, a passthrough mode is being added. That way we can pass pino messages to pino-gelf and then
pipe pino-gelf's output to another tool. That way we can both send to a graylog server and pretty-print.
@jfahrenkrug
Copy link
Contributor Author

@mcollina Ping :)

README.md Outdated
@@ -27,7 +27,7 @@ The recommended pipeline to run Pino GELF as a transform for Pino logs is as fol
node your-app.js | pino-gelf log
```

The host, port and maximum chunk size of your Graylog server can be specified using options. Options can also be used to include standard fields logged by [express-pino-middleware](https://github.com/pinojs/express-pino-logger). Custom fields included with Pino requests based on your own configuration can also be specified, although at present only strings are handled. Finally, you can choose to enable verbose mode which outputs all GELF messages locally, this can be useful for initial configuration.
The host, port and maximum chunk size of your Graylog server can be specified using options. Fields logged by [express-pino-middleware](https://github.com/pinojs/express-pino-logger) or any other custom fields included with Pino requests will be converted into strings (if they aren't already) and their keys will be prefixed with an underscore, according to the GELF spec. Also, you can choose to enable verbose mode which outputs all GELF messages locally, this can be useful for initial configuration. Finally, you can enable passthrough mode which will output the original input back to stdout. That enables you to make `pino-gelf` part of a chain of commands and use the original input for further processing (for example `pino-pretty`).
Copy link
Member

Choose a reason for hiding this comment

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

this has nothing to do with gelf correct? this is just an additional convenience feature? does anything else happen when passthough mode is enabled - e.g. is it still sending data somewhere?

Copy link
Member

@davidmarkclements davidmarkclements Jul 5, 2019

Choose a reason for hiding this comment

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

I think maybe passthrough should go in its own heading

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidmarkclements Yes, putting passthrough in its own heading is a good idea, I'll take care of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidmarkclements I've decided not to put it into its own heading but into its own paragraph instead. It's still under "usage" where I believe it belongs. And I had already bumped the version up to 2.0.0.

Copy link
Member

Choose a reason for hiding this comment

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

awesome

Copy link
Member

@davidmarkclements davidmarkclements 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 not familiar with gelf - would this be a new major version?

@pastelsky
Copy link

Bump. Can this be merged? This also fixes #3

@jfahrenkrug
Copy link
Contributor Author

I'm not familiar with gelf - would this be a new major version?

I think you are right, it could warrant a new major version because this PR changes the output which could potentially be a breaking change is some setups. I can go ahead and bump the version number.

@davidmarkclements
Copy link
Member

@jfahrenkrug would you say this is ready to go?

@jfahrenkrug
Copy link
Contributor Author

@jfahrenkrug would you say this is ready to go?

@davidmarkclements I think so. The version in the package.json is currently set to ""2.0.0-beta.0". Do you want to leave it this way and release a beta first? I'm not sure how many people use this package. If it's a small number, we could probably skip the beta phase and go straight to "2.0.0".

@davidmarkclements
Copy link
Member

happy to skip beta

@jfahrenkrug
Copy link
Contributor Author

happy to skip beta

@davidmarkclements Excellent! I just changed the version to "2.0.0", so it's ready to be merged now.

@jfahrenkrug
Copy link
Contributor Author

@davidmarkclements I don't have write access, would you be able to merge it?

@mcollina mcollina merged commit 9df316a into pinojs:master Aug 23, 2019
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.

4 participants