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

New Pipe Reporter Interface #396

Closed
arb opened this issue Nov 1, 2015 · 11 comments
Closed

New Pipe Reporter Interface #396

arb opened this issue Nov 1, 2015 · 11 comments
Assignees
Milestone

Comments

@arb
Copy link
Contributor

@arb arb commented Nov 1, 2015

I'm proposing that the new Good reporter interface allow users to pass an array of streams. Good would then create a stream pipeline with these streams. This would allow users to do things like use different filtering logic, clone the payloads before passing them on to other streams, and add additional implementation specific information.

I'm proposing the reporters option would look something like this:

{
  reporters: 
    'awesome_log-reporter': {
      streams: [{
        module: 'good-squeeze',
        constructor: 'Squeeze'
        args: [{ log: '*', error: ['debug', 'internal' ]}]
      }, {
        module: 'good-file',
        args: ['./test/fixtures/awesome_log']
      }]
    },
    'log-hg-received-time': {
      streams: [{
        module: 'good-squeeze',
        constructor: 'Squeeze'
        args: [{ ops: '*' }]
      }, {
        module: './lib/clone-data'
      }, {
        module: 'high-res-time',
        args: [{ timezone: 'EST', legacyMode: false, key: '__hrReceived' }, 'round-down']
      }, {
        module: 'good-file',
        args: ['./test/fixtures/awesome_log']
      }]
    }
}

The reporters key will be a key/value pair. The key will be user friendly name that this reporter will be referred to. The value will be another object that has a streams array. Each item in the streams array is an object. The module value is either a path to a stream or a npm module to a stream object. constructor is an optional string in cases where the stream constructor isn't the result of require('module') and is the function used to create a new stream. Finally, args is an array of values passed to the constructor specified by module. Each object inside the reporter object represents a stream object. When good starts, it will pipe each stream in the order of the items in the array, one into the next using pumpify.

This will give good users much more control over how things are filtered, adding additional information to log messages, cloning, and other destination options.

@gergoerdosi

This comment has been minimized.

Copy link
Contributor

@gergoerdosi gergoerdosi commented Nov 1, 2015

I was about to create something similar, so +1. Two questions:

  1. Pumpify looks a bit outdated, it is tested only on node 0.10. We should either contribute to it, or make a similar module under the hapijs organization. What do you think?
  2. How do you see the ecosystem? People will create higher level modules (eg. "awesome_log-reporter") or they will create lower level streams (eg. "good-file") that are then glued together in hapi?
@arb

This comment has been minimized.

Copy link
Contributor Author

@arb arb commented Nov 2, 2015

  1. Why does it look outdated? Hasn't had significant updates in a while, but I don't think it really needs updating that often. I've met Mr. mafintosh at NodeConf and I don't know of anyone who knows more about streams than him. We could post an issue on there though asking about the long term status of the project though.
  2. The ecosystem would be that you just create transform streams that good glues together with pumpify. They would all have to be in "object mode", but other than that, any existing transform stream could be used as long as it sticks to the basic interface.
@gergoerdosi

This comment has been minimized.

Copy link
Contributor

@gergoerdosi gergoerdosi commented Nov 2, 2015

  1. There have been many changes to streams since v0.10. Most of them are non-breaking, but I would like to make sure the code that was written for v0.10 also works on v4 and v5 (and later). Updating .travis.yml with the new versions should be enough if the code works.
  2. Sounds good! Just a note related to this. Streams will be independent and as you said they can modify objects by cloning. Would it be possible to pass both the original (frozen) and the modified objects to streams? The original could be used to get data from, the modified could be used as the final one that gets logged.
@arb

This comment has been minimized.

Copy link
Contributor Author

@arb arb commented Nov 2, 2015

  1. Yeah I agree, we would want to verify that. I would like to avoid taking on the baggage of the stream library if possible. Every effort should be used to try to keep that stuff in user land IMO. Unless a stream master wants to tackle that and have it within the hapijs universe.
  2. The signature to the stream is always like chunk or data; a single argument so I guess we would have to wrap everything up into a single massive argument to do that. That's the only way to keep the API in line with any existing streams.
@gergoerdosi

This comment has been minimized.

Copy link
Contributor

@gergoerdosi gergoerdosi commented Nov 2, 2015

True, I forgot that. I'm open to anything, I just would like to avoid the case when one stream breaks because the other modifies the object.

@devinivy

This comment has been minimized.

Copy link
Member

@devinivy devinivy commented Nov 2, 2015

I see no particular reason to recreate any of those "canonical" stream modules (generally, the mississippi modules), but I can think of no reason that they shouldn't be tested on recent versions of node. Looks like pumpify will work fine on node v4 and v5: https://travis-ci.org/devinivy/pumpify/builds/88802545 .

@arb

This comment has been minimized.

Copy link
Contributor Author

@arb arb commented Nov 2, 2015

I guess first steps would be to open a PR there and see if we can get some better testing on newer versions of Node there.

@arb arb modified the milestone: 7.0.0 Nov 18, 2015
@arb

This comment has been minimized.

Copy link
Contributor Author

@arb arb commented Nov 24, 2015

Update I'm thinking that the interface should be an array of streams, and then another key that represents the end of the stream. This is so that the ends of the streams can implement an asynchronous stop method. This would be useful for tear downs.

@arb arb self-assigned this Feb 24, 2016
arb added a commit to arb/good that referenced this issue Feb 29, 2016
@arb arb closed this in 75f35fc Mar 1, 2016
@fluky

This comment has been minimized.

Copy link

@fluky fluky commented May 2, 2016

I'm having trouble with this change. Using glue this is how I registered a custom reporter:

            plugin: {
               register: 'good',
               options: {
                  reporters: [{
                     reporter: reporter,
                     events: {
                        error: '*',
                        log: '*',
                        ops: '*',
                        request: '*',
                        response: '*' 
                     }   
                  }]  
               }   
            }   

I'm struggling to find something that works with the new interface. Everything I try gives me errors. Looking at the example in the first post I would assume I would use:

            plugin: {
               register: 'good',
               options: {
                  reporters: {
                     'my-stream': {
                        streams: [{
                           module: require ('../src/server/reporters/mongo'),
                           args: [{
                              error: '*',
                              log: '*',
                              ops: '*',
                              request: '*',
                              response: '*' 
                           }]  
                        }]  
                     }   
                  }   
               }   
            }   

But that errors with: Error: Invalid monitorOptions options child "reporters" fails because [child "my-stream" fails because ["my-stream" must be an array]]

If I make it an array (it's not in the example) I get: Error: Invalid monitorOptions options child "reporters" fails because [child "my-stream" fails because ["my-stream" at position 0 does not match any of the allowed types]]

@arb

This comment has been minimized.

Copy link
Contributor Author

@arb arb commented May 3, 2016

This issue was closed two months ago. If you are having problems, please open a new issue and don't bump old ones. That being said, you're probably using version 6 of good and not 7.

Either way, streams doesn't seem to be a valid option to either good 6 or good 7.

@fluky

This comment has been minimized.

Copy link

@fluky fluky commented May 3, 2016

No I'm using 7 and 7 was released with this breaking change 11 hours ago, but thanks so much for the help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.