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

Discussion around winston-papertrail 2.x #84

Open
ffxsam opened this issue Nov 9, 2018 · 17 comments
Open

Discussion around winston-papertrail 2.x #84

ffxsam opened this issue Nov 9, 2018 · 17 comments

Comments

@ffxsam
Copy link
Collaborator

ffxsam commented Nov 9, 2018

It seems this project is no longer maintained. Is that the case? @kenperkins @troy

@troy
Copy link
Contributor

troy commented Nov 9, 2018

I'm no longer a maintainer nor using it personally, but @johlym or @markdascher may know the current status.

@ffxsam
Copy link
Collaborator Author

ffxsam commented Nov 9, 2018

I'm considering starting a fork. I have a need to have separate/multiple log formatters in a single transport, or at least a way to internally share a single connection so that multiple instances don't open new connections.

But if I can open a PR, that would be preferable. I just want to make sure it could actually get merged by a maintainer.

@kenperkins
Copy link
Owner

I'm amenable to moving the repo if the current maintainers are unavailable. I'm also willing to take new maintainers. I'm just unable at present to commit any time and am far behind on the current state.

@ffxsam
Copy link
Collaborator Author

ffxsam commented Nov 9, 2018

More on my use case:

I'm using the logFormat option to add in my own metadata next to the log level, e..g:

/* Inside my Logger class constructor */
    const logFormatter = (level, message) => {
      return `[${level}] ${this.metadataString} ${message}`;
    };

The Logger class constructor starts off with this:

class Logger {
  constructor(programName, metadata) {
    this.programName = programName;
    this.metadataString = metadata
      ? Object.keys(metadata)
          .filter(key => !!metadata[key])
          .map(key => `[${key}:${COLOR_RED}${metadata[key]}${COLOR_END}]`)
          .join(' ')
      : '';

So basically, in Papertrail I get log lines that look like:

Nov 09 08:43:22 ec2-async-handler-main async-worker-staging:  [info] [pubId:3xc7KB9QyAR3KF4CT] Calling getcountasync 

because I can instantiate a logger like so:

const logger = new Logger('async-worker-staging', {
  pubId: '3xc7KB9QyAR3KF4CT',
});

The thing is, we're working with massive amounts of data, and we go through a huge loop where that pubId will be different every time (meaning, a separate log formatter function every time). I've built my Logger class in a way where it maintains an internal map of programs, and only creates one transport per program name, so as not to open potentially hundreds of connections to Papertrail. Of course, this also means a single log formatter per transport, which doesn't work for this use case. And I can't log the pubId in the usual metadata arg:

logger.log('info', 'Blah', { pubId: '3xc7KB9QyAR3KF4CT' })

because there's dozens of lines of other metadata, and I need that pubId on every single log line. If it's in the metadata as shown above, it only shows up on one line.

I don't know how much of that makes sense, it's hard for me to explain (code is better shown than talked about). 😉 If there's some easy way to do this, I'm all ears because I'd rather not spend much time on it if I don't have to!

@ffxsam
Copy link
Collaborator Author

ffxsam commented Nov 9, 2018

It seems like the winston Papertrail class could be modified to store a reference to the socket connection statically on the class, and so future instantiations of winston.transports.Papertrail could check and reuse the existing connection (if the host and port match, of course). So an internal map of sockets would exist, e.g.

{
  "logsX.papertrail.com:12345": conn1,
  "logsX.papertrail.com:98765": conn2
}

I dunno if that would work or is a good idea. Admittedly I'm rather new to both winston & Papertrail.

@ffxsam
Copy link
Collaborator Author

ffxsam commented Nov 9, 2018

@kenperkins @johlym @markdascher I'm working on a rewrite of the Papertrail transport. It'll be a new major version since it introduces breaking changes (built for winston 3.x). Right now this is just for my own internal use, but once I have a chance to polish up the code, document it, write tests (it's gonna be a while), I can open a PR and have some folks look at it.

One big departure in functionality will be the ability to have multiple transports use a single connection, e.g.:

const connection = new PapertrailConnection({
  host: 'logsX.papertrailapp.com',
  port: 12345,
});
const papertrailTransport = new PapertrailTransport({
  connection,
  colorize: true,
  program: 'test',
  logFormat: logFormatter,
});

This will allow people to use different log formatting and different program parameters, without creating multiple connections to Papertrail.

I'll use this issue to post updates.

@ffxsam ffxsam changed the title Dead project? Discussion around winston-papertrail 2.x Nov 9, 2018
@ffxsam
Copy link
Collaborator Author

ffxsam commented Nov 12, 2018

@kenperkins Hi Ken, could you please add me as a maintainer?

Also, does anyone have any spare cycles to review the code once I've opened a PR for 2.x? It's a drastic departure from 1.x, and I'm not entirely sure how I can force socket errors to test & ensure all of that works correctly.

@ZacharyDraper
Copy link

@ffxsam I am interested in your rebuild. Tried this today with Winston 3.1.0 and while it works, it has a number of problems. Would love to help test what you are working on.

@ffxsam
Copy link
Collaborator Author

ffxsam commented Dec 11, 2018

@ZacharyDraper Great, I could use the help as I'm swamped lately! The code isn't pushed yet, it's on my Mac which is not hooked up at the moment. I'll post here when it's up so people can contribute.

@lcersly
Copy link

lcersly commented Dec 12, 2018

@ffxsam Also interested in the rebuild. Especially the part about single connection but multiple loggers. In Winston 3.0 they are using something called categories, and it sound like something I would like to use if it's supported by Papertrail.

As such, I would like to help with either reviewing or rewriting if need be.

@ffxsam
Copy link
Collaborator Author

ffxsam commented Dec 31, 2018

Hey all! v2 branch is up: https://github.com/kenperkins/winston-papertrail/tree/v2

The only change at the moment is lib/winston-papertrail.js. There's still a bit that needs to be done here, such as handling failed connections and such. But on a basic level, it works. Here's an example:

const connection = new PapertrailConnection({
  host: 'logsX.papertrailapp.com',
  port: 123,
});
const papertrailTransport = new PapertrailTransport({
  connection,
  colorize: true,
  program: 'hello',
  logFormat: (level, message) => `[${level}] ${message}`,
});

const logger = winston.createLogger({
  transports: [papertrailTransport],
  format: winston.format.colorize(),
});

And then:

logger.log({
  level: 'info',
  message: 'Log THIS!',
});

Please feel free to open PRs against the new v2 branch. Please make sure all ESLint rules pass.

@ZacharyDraper
Copy link

@ffxsam I did some testing on this today and it works great! It solves all of the problems that were keeping us from using this package. Thank you!

@ffxsam
Copy link
Collaborator Author

ffxsam commented Jan 21, 2019

No prob! Keep in mind it's still incomplete as it doesn't handle network errors gracefully (or at all). If anyone can help contribute, that'd be awesome.

@ZacharyDraper
Copy link

ZacharyDraper commented Jan 21, 2019

@ffxsam What ideas do you have on what should happen in the event a connection cannot be made to Papertrail? The following code at the bottom of your connect() method would at least be able to detect the problem. For now, I have it just printing a console message.

    socket.on('error', function(e){
        console.log('Unable to connect to Papertrail');
    });

In testing this, I've found that Papertrail seems to become unavailable every night for a short time and that when the transport is unable to connect it simply stops running. Even when Papertrail is available, the transport will not send log messages until it is restarted. Will continue to investigate and update this post.

@draperunner
Copy link

@ffxsam I opened #86 to enable the inlineMeta option, which was not set in the constructor, therefore always ignored.

@b5l
Copy link

b5l commented Apr 2, 2019

We would like to switch from log4js to winston 3 in our software, however we need winston-papertrail for it. Releasing a version to our clients with a git repository in its package.json is not an option. Can we expect version 2 of winston-papertrail to be released soon?
What has to be done before it can be released? I might be able to contribute.

@ffxsam
Copy link
Collaborator Author

ffxsam commented Apr 2, 2019

@benjaminlaib There's a working v2 branch, you're welcome to use it and work on it. It needs more comprehensive error-handling, I think. Other than that, it works.

Personally, it turns out I won't be using Papertrail in the future, so I won't be contributing. But I'll be around for any PRs into the v2 branch.

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

No branches or pull requests

7 participants