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

Roadmap and Ideas #292

Closed
arb opened this issue Jan 8, 2015 · 10 comments
Closed

Roadmap and Ideas #292

arb opened this issue Jan 8, 2015 · 10 comments

Comments

@arb
Copy link
Contributor

@arb arb commented Jan 8, 2015

So I have a few ideas about next version, but I wanted to run some of them by the users before I went and did it. I am actively looking for feedback on these suggested changes.

1.Make all reporters just streams and pipe into them from good
The thought is all of the reports are basically one step away from just being streams, so lets just take it the next step and make them streams. It is closer to the "node way" and streams basically exist to move data from point A to point B which is all we are doing here. In the reporter start function, you would be handed the "good main pipe" and you would set up your pipeline in the start function. I think there would be a standard transform stream available in GoodReporter that would do the filtering for you. This will be a hard breaking change because all of the base reporters will have to be updated. We can do RC candidates to address this so it isn't as painful.
2. Remove the good-reporter base class
I don't think it does much for you and it's just another moving piece, especially with the change proposed in number 1. Plugins and catbox don't work this way; so there isn't really a great reason good-reports need to. I think good-reporter will be repurposed to only include utility methods like a transform stream that will encapsulate the filtering logic so every pipeline can use the same good-reporter filter stream (if they want to).
3. Customize event registration
In addition to the main 4 or 5 events we are already using, I was thinking of adding an option to listen for additional events ("start" and "stop" come to mind [semi-related issue here). The rub is that they would receive the default data that hapi sends, nothing customized like the existing good event payloads. This would let you have more control over the events you get without needing to constantly update good.

@ulrikstrid

This comment has been minimized.

Copy link

@ulrikstrid ulrikstrid commented Jan 11, 2015

In general I think these are great ideas and would make good even more flexible.

Would number 2 make it so that the implementations of the reporters need more boilerplate as we have to implement what good-reporter gives us now?

@arb

This comment has been minimized.

Copy link
Contributor Author

@arb arb commented Jan 11, 2015

The amount of boilerplate will probably end up being the same and possible a little less. You'll need a start, stop, and a constructor type function and that's pretty much it.

All good-reporter was doing was setting up some _ values and doing the event filtering based on tags and event type. The filtering will be a transform stream exposed by good-reporter so that logic will still be reusable.

@fhemberger

This comment has been minimized.

Copy link
Contributor

@fhemberger fhemberger commented Jan 13, 2015

As the issue already arose on single reporter implementations: Wouldn't it be a good idea to make the event payload data immutable (via Object.freeze()), just to be sure that no reporter can accidentally modify it and pass it on to the next reporter?

@arb

This comment has been minimized.

Copy link
Contributor Author

@arb arb commented Jan 13, 2015

Yeah this already happens here for example. But yes I agree, the new ones should be frozen as well.

@yonjah

This comment has been minimized.

Copy link

@yonjah yonjah commented Feb 5, 2015

I have a few more suggestions.

  1. Allow adding/removing/changing reports while app is running
    Today once you initialize Good all your setting are fixed and you cannot add / remove reporters or change the event/log level. add an API that will allow to either replace a running reporter or better yet change the events its logging. This is highly useful when you have a production APP misbehaving, and you want to increase the log level to try and debug what might be wrong with it.
  2. Make sure stopping a reporters can actually be stopped
    To get the dynamic functionality one of the main thing that's missing in reporters today is the fact
    that the start function bind a listener to that reporter but the stop function doesn't remove that listener.
    so once you started a reporter its basically logging your app no matter what (Today its not a common problem since any way the only time the stop method is being called is once the server stop)

I created a good-magic reporter that will allow me to do what I'm describing with the current API. its kinda hackish now because of the current API limitation but I would much more rather see that as a built in option in Good.

@yonjah

This comment has been minimized.

Copy link

@yonjah yonjah commented Feb 5, 2015

If I can suggest one more thing.
After looking at good-console code, if Good is going to use Streams which seems like a great Idea, it should probably follow the Do one thing methodology.
So If I use good-console as an example it should be divided
to 3 different units each with its own stream

  1. filter - to filter custom events, this seems kinda weird to be
    part of the reporter since it probably more related to the way Hapi server emits events
    and less for a specific reporter, yet if we think the filter needs to be customized
    it should be separated from the reporter.
  2. formatter - formatters can be generic across different reporters so we might have a JSON formatter
    or a text formatter or csv formatter
  3. reporter - the reporter that actually display/save the logs in the case of good-console this is just process.stdout, But for more complex reporters that will be a custom Stream that might save to a specific file or sends it throe the network.

So if we are following that logic we might need to call good like that -

server.register({
        register: good,
        options: {
            pipes: [{
                    filters: [{
                        filter: require('good-event-filter'),
                        args: [{log: '*'}]
                    }],
                    formatters: [{
                        formatter: require('good-unixColor-formatter')
                    }],
                    reporters: [{
                            reporter: require('good-console-reporter')
                        }, {
                            reporter: require('good-file-reporter'),
                            args:['/var/log/myapp.log']
                    }]
                }]
        }
}, function (err) {
    if (err) {
        throw err;
    }
    server.start(function () {
        server.log('info', 'Server started at ' + server.info.uri);
    });
});

So if some one for some reason runs node on a windows server he can
change formatter: require('good-unixColor-formatter') to formatter: require('good-windowsColor-formatter')
since I assume window has different way to change colors in the terminal.

this can be kinda annoying to manage so many dependencies so it might be nice if good would come with
many of the basic built in and probably will allow to define them with just strings something like

server.register({
        register: good,
        options: {
            pipes: [{
                    filters: [{
                        filter: 'event',
                        args: [{log: '*'}]
                    }],
                    formatters: 'unixColor',
                    reporters: [ 'console', {
                        reporter: 'file',
                        args:['/var/log/myapp.log']
                    }]
                }, {
                    filters: [{
                            filter: 'event',
                            args: [{error: '*'}]
                        }, {
                            filter: require('good-regex-filter'),
                            args: [{data: /^argent/}]
                        }
                    ],
                    formatters: 'html',
                    reporters: [{
                        reporter: require('good-mail-reporter'),
                        args:[{to: 'me@me.com', subject: 'Argent Error'}]
                    }]
                }
            ]
        }
}, function (err) {
    if (err) {
        throw err;
    }
    server.start(function () {
        server.log('info', 'Server started at ' + server.info.uri);
    });
});
@arb

This comment has been minimized.

Copy link
Contributor Author

@arb arb commented Feb 5, 2015

@yonjah

Currently, the reports aren't streams, they are pre-built pipelines. So they technically still follow the do one thing rule. I did consider your approach of just supplying pipe information but I think that level of config would become very annoying over time. Also, where would custom logic go? good-file needs something "around" the pipes to manage the rotation options. If you want to be able to update reporters as you've suggested, you'd need something there to contain that logic as well. If the reporters were "logic-less" I agree, that would be a great way to do it, but they aren't.

I addressed your number two in the related issue. If you need special handling in stop, you need to override both start and stop. In the future there will just be a "stop" event emitted that can handle shut down. Though there is an issue with async shut downs in hapi per hapijs/hapi#2389. Also, stop isn't really intended to be used to stop and start the reporter on demand. It's only ever called when the server object controlling good is being stopped and the node process is likely about to be shut down. So it doesn't really need handles to existing event handlers.

It's unlikely that good will come with anything pre-packaged again. That caused way more problems than it solved. I could see putting common pipes in good-squeeze though rather than just the event filtering one.

Your number 1 is a good idea though and I'll see what can be done about it maybe not initially, but as an enhancement down the road.

@yonjah

This comment has been minimized.

Copy link

@yonjah yonjah commented Feb 5, 2015

@arb
Why can't the pipe have logic in them ? the same way as they have logic now
only instead of being in one place it will be divided to 3 different places

even if for now before we can think of the a good way to make the config more simple to use
I would make each reporter export 3 different streams filter, formatter, reporter and let good monitor pipe them together. this will make it much more easy and straight forward in the future to find a way to mix and match this logic which is now glued together.

About my second point. I think you keep missing it.
I agree completely each implementor needs to implement his own stop method.
But if you look at the code of all the Goodreporters on npm ( and I've checked them all) none of them does it properly here is an example from good-udp

internals.GoodUdp.prototype.start = function (emitter, callback) {
    emitter.on('report', this._handleEvent.bind(this));
    return callback(null);
};
internals.GoodUdp.prototype.stop = function () {
    this._sendMessages();
};

You can can see it overrides both the start and stop methods but completely fails to unregister the report listener so if by a magical way stop will be called but the emitter will still be active
and will call _handleEvent with logs.
(If I'm still don't explain it properly look at my comment in hapi-magic and the following start stop implementation I hope the code might make it clearer)
I think the reason no other reporter has this right is because the base class doesn't do it and so it hard to understand that this actually need to be done.
This can be left broken in the current API if its being deprecated, but we need to notice this doesn't leak to the new API

@arb arb closed this Apr 7, 2015
@arb

This comment has been minimized.

Copy link
Contributor Author

@arb arb commented Oct 25, 2015

@yonjah, I'm reconsidering your proposal after all this time and learning about the pumpify module.

@yonjah

This comment has been minimized.

Copy link

@yonjah yonjah commented Oct 29, 2015

@arb cool

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.