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

add loggly support #64

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cedricwalter
Copy link

@cedricwalter cedricwalter commented Dec 18, 2017

add loggly support using npm "winston-loggly-bulk"

NOTES

  • Files graylogFormat.js & logglyFormat.js are identical, could be renamed to keyvaluejson.js as both share the same data structures (json)
  • only command line parameters are for now supported, no watch.yml, but i am open to do this change
  • A file forwarder (OS specific) could also be used instead of doing code changes, but this PR is more portable and run as long rpm run on your OS

JSON is send to Loggly using winston transport, console transport still log using stringify (now an option in constructor)

Feedback welcome!

@codecov
Copy link

codecov bot commented Dec 18, 2017

Codecov Report

Merging #64 into master will decrease coverage by 0.63%.
The diff coverage is 70.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
- Coverage   74.66%   74.02%   -0.64%     
==========================================
  Files          11       11              
  Lines         371      385      +14     
==========================================
+ Hits          277      285       +8     
- Misses         94      100       +6
Impacted Files Coverage Δ
src/command.js 96.29% <100%> (+0.21%) ⬆️
src/logger.js 57.69% <64.28%> (-8.98%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b1163fa...2fb6f35. Read the comment docs.

Copy link
Member

@Moejoe90 Moejoe90 left a comment

Choose a reason for hiding this comment

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

Thanks for this PR

a couple of points

  • It's better to keep this tool OS agnostic
  • Add options to both env and .watch config
  • as you mentioned in point 2 let's use a general JSON-format since both graylogformat, and logglyformat are the same I can see that repeating for other services.
  • Solve merge conflicts
  • ES-lint

Other than that it will be a good addition

setLoggerLevel(logLevel);
setLogglyTransport(logglyAccessToken, logglySubDomain, logglyTag);
Copy link
Member

Choose a reason for hiding this comment

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

Can we set a conditional check and only setLogglyTransport when the output type is chosen as loggly. as it doesn't make sense to always set it up

import { getCommandVars } from './command';

require('winston-loggly-bulk');
Copy link
Member

Choose a reason for hiding this comment

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

I understand that require is more natively by node but let's stay with the code and use import

const loggerConsoleOptions = {
timestamp: false,
colorize: false,
formatter: options => `${options.message}`,
json: true,
Copy link
Member

Choose a reason for hiding this comment

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

What will happen if type of output chosen was terminal?

transports: [
new (winston.transports.Console)(loggerConsoleOptions),
] });
let logger;
Copy link
Member

Choose a reason for hiding this comment

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

Why is it not constant?


export default (data, type = 'terminal') => {
switch (type) {
case 'terminal':
logger.log('info', terminalFormat(data));
break;
case 'graylog':
logger.log('info', JSON.stringify(grayLogFromat(data.transaction,
data.decodedInputDataResult, data.decodedLogs)));
logger.log('info', grayLogFromat(data.transaction,
Copy link
Member

Choose a reason for hiding this comment

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

I agree with your point, let's make them both jsonFormat

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.

2 participants