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

contrib: skeleton app structure & Dogstatsd nsqd addon #909

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

Conversation

dm03514
Copy link

@dm03514 dm03514 commented Jun 19, 2017

The goal of this is to add a contrib pacakge which allows for extensions/addons for NSQD. The idea was to make the contrib as separate from NSQD as possible. To do this the contrib nsqd addons are passed a reference to NSQD and can only interact with it through its exported methods.

It was also the hope that contrib addons would be completely segregated from core so that updating implementation or command line flags shouldn't require touching core at all.

DatadogStatsd was chosen to deliver the contrib package.

  • Get Datadogstatsd actually talking to local statsd :(
  • Cleanup flags - (a single multi-value (array) flag)
  • Dogstatsd test coverage
  • Have default ddstatsd options be defined in contrib/dogstatsd.go instead of contrib/nsq.go
  • AddModuleGoroutine() && Exit channel passed to Loop
  • README on how to add contrib modules
  • contrib/nsqd.go make sure that options are provided for each module initialized
  • update contrib/nsq.go to filter out valid opts and only pass those to the addon initializer

refs #904

Verified Datadogstatsd can communicate with local udp server

Started with:

$ ./apps/nsqd/nsqd --mod-opt=-dogstatsd-address=127.0.0.1:8125

Verified in local UDP echo server:

waiting to receive message
received 72 bytes from ('127.0.0.1', 55315)
nsq.channel.in_flight_count:0|g|#topic_name:name,channel_name:super_cool
sent 72 bytes back to ('127.0.0.1', 55315)

waiting to receive message
received 71 bytes from ('127.0.0.1', 55315)
nsq.channel.deferred_count:0|g|#topic_name:name,channel_name:super_cool
sent 71 bytes back to ('127.0.0.1', 55315)

waiting to receive message
received 70 bytes from ('127.0.0.1', 55315)
nsq.channel.requeue_count:0|c|#topic_name:name,channel_name:super_cool
sent 70 bytes back to ('127.0.0.1', 55315)

waiting to receive message
received 70 bytes from ('127.0.0.1', 55315)
nsq.channel.timeout_count:0|c|#topic_name:name,channel_name:super_cool
sent 70 bytes back to ('127.0.0.1', 55315)

* contrib package with nsqd contrib
* skeleton interfaces
* skeleton datadogd contrib package
* explicitly adds opts to nsqd
nsqd/options.go Outdated

// contrib
// TODO cleanly extend this in the the contrib app
DogStatsdAddress string `flag:"dogstatsd-address"`
Copy link
Author

@dm03514 dm03514 Jun 19, 2017

Choose a reason for hiding this comment

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

Not sure how do do this cleanest because brand new to go. Thinking of NSQDContribOptions that embeds NSQD Options, contrib options will have an option struct for each contrib addon. The goal is to not touch nsqd core when create ing new addons.

daniel mican added 3 commits June 19, 2017 12:27
* addons are instantiated with reference to nsqd
* nsqd exposes exit channel reference
* datadog contrib skeletal loop
@dm03514 dm03514 changed the title contrib: skeleton app structure [WIP] contrib: skeleton app structure Jun 19, 2017
* datadog client in contrib
* statsd topic/channel stats
@ploxiln
Copy link
Member

ploxiln commented Jun 19, 2017

I was thinking these things would be called something like "optional modules". Though "contrib" isn't too bad.

My idea for command-line flags, and the config file, is to pass through a single multi-value (array) flag, which would be called something like --mod or --mod-opt. It would be used like:

... --mod=ddstatsd=address=127.0.0.1:2345 --mod=ddstatsd=interval=30

The main "module" or "contrib" system would only initialize a "module" if there are any options for that module. It passes the module just that module's options as an array like ["address=127.0.0.1:2345", "interval=30"].

This way, everything about a module's options is only in the module, and nsqd flag and config file handling is unaffected except for a single --mod that collects an array of strings and just passes them along.

Just my humble ideas :)

contrib/nsqd.go Outdated

// Starts all addons that are active
func (as *NSQDAddons) Start() {
logger.Println("Starting All addons")
Copy link
Member

Choose a reason for hiding this comment

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

This should somehow go through NSQD.logf() too.


// would like to expose logf to the contrib modules so that contrib can share the
// configuration end user specifies on CLI
func (n *NSQD) Logf(level lg.LogLevel, f string, args ...interface{}) {
Copy link
Member

Choose a reason for hiding this comment

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

A possibly cleaner alternative is to pass the logf bound-method reference to NewNSQDAddons() or NSQDAddons.Start(), which could take a internal.lg.AppLogFunc type. For example see

protocol.TCPServer(n.tcpListener, tcpServer, n.logf)
and
func TCPServer(listener net.Listener, handler TCPHandler, logf lg.AppLogFunc) {

@dm03514
Copy link
Author

dm03514 commented Jul 17, 2017

@ploxiln with the mod-opts flag and pushing the opts parsing to the optional modules, do you know of any way to show the optional modules CLI options on the top level nsqd --help ? Is that a requirement?

daniel mican added 3 commits July 17, 2017 12:35
* exposes single new option on nsqd
* pushes option handling down to each individual optional module
@ploxiln
Copy link
Member

ploxiln commented Jul 17, 2017

Yeah, it won't be convenient to show the module options in nsqd --help, that's a downside. But you could add a --mod-opts-help flag to call into each module to print its options ... just an idea.

@dm03514
Copy link
Author

dm03514 commented Jul 17, 2017

@ploxiln awesome! ty

@ploxiln
Copy link
Member

ploxiln commented Jul 17, 2017

@mreiferson - have any opinions on this, the structure it is developing?

@@ -220,6 +225,7 @@ func (p *program) Start() error {
cfg.Validate()

options.Resolve(opts, flagSet, cfg)

Copy link
Member

Choose a reason for hiding this comment

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

best not to add unrelated whitespace changes here

nsqd/nsqd.go Outdated
@@ -265,6 +269,10 @@ func (n *NSQD) Main() {
}
}

func (n *NSQD) RegisterAddon(addonFn func()) {
n.waitGroup.Wrap(func() { addonFn() })
Copy link
Member

Choose a reason for hiding this comment

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

could this possibly just be written as n.waitGroup.Wrap(addonFn) ?

Copy link
Member

@ploxiln ploxiln Jul 17, 2017

Choose a reason for hiding this comment

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

I might name RegisterAddon() slightly differently, like AddModuleGoroutine().

Here's another idea - to avoid needing to add the new public ExitChan() method to NSQD, pass the exitChan to the coroutine function - they'll all need it to integrate with the wait group anyway.

(That'll require going back to creating a trivial wrapper function like n.waitGroup.Wrap(func() { modFn(&n.exitChan) }))

* stubbed out more opts related tests
* removed modified whitespace from nsqd.go
@@ -232,6 +238,10 @@ func (p *program) Start() error {
}
nsqd.Main()

// hook into addons
addons := contrib.NewEnabledNSQDAddons(opts.ModOpt, nsqd, nsqd.Logf)
Copy link
Member

Choose a reason for hiding this comment

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

Eh, well, not much point in passing the public method Logf separately.

Imagine if something like addons.Start() was called from within NSQD.Main(), then it could pass n.logf as a parameter to that, and avoid adding the new public method. But I understand that won't be very convenient or clean, so I don't have a better idea at the moment ...

Copy link
Author

Choose a reason for hiding this comment

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

I moved the addon initialization to Main() after your initial comment and passed lg.AppLogFunc to the Addons, but then there was a circular dependency :p . The only thing I could think of was to add an interface to represent NSQD? Can you think of any easier ways?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that makes sense ... nope, can't think of a better way at the moment.

These "contrib addons" are going to want to get information from the NSQD, and maybe some will want to make changes, e.g. to the list of nsqlookupd, or auth info for new connections. These various purposes will need some functions to be made public, which we'll consider on a case-by-case basis. An interface of these public functions which contrib modules can use sounds like it could be a pretty good idea.

contrib/nsqd.go Outdated

// ask each addon if it should be initialize
dogStats := NewNSQDDogStatsd(contribOpts, n, logf)
if dogStats.Enabled() {
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to only call NewNSQDDogStatsd() if there are any contribOpts that start with "dogstatsd=" - that way, you can be sure that any "contrib modules" which you did not pass options to did not run anything, without looking at their code.

So this function would just need a little table of name strings and init functions to loop over.

@mreiferson
Copy link
Member

Sorry I'm late following up on this — I feel like we should investigate how we could structure this using https://golang.org/pkg/plugin/. It would be linux-only, but it would allow us to define some sort of API in nsqd and then support runtime-loaded plugins that could be maintained separately, e.g. this datadog stats plugin.

@ploxiln
Copy link
Member

ploxiln commented Jul 23, 2017

Eh, I think it's a bit early to use Plugin

contrib/nsqd.go Outdated

// ask each addon if it should be initialize
dogStats := NewNSQDDogStatsd(contribOpts, n)
if dogStats.Enabled() {
Copy link
Member

Choose a reason for hiding this comment

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

(this comment copied to a new one because the original was hidden by github)

I think it would be better to only call NewNSQDDogStatsd() if there are any contribOpts that start with "dogstatsd=" - that way, you can be sure that any "contrib modules" which you did not pass options to did not run anything, without looking at their code.

So this function would just need a little table of name strings and init functions to loop over.

Copy link
Author

Choose a reason for hiding this comment

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

Is this a flexible requirement? Is there ever a case where a contrib module
flag could be specified
(ie dogstatsd=prefix) but not qualify as enabled? I feel like since the
flags are encapsulated in specific contrib module, delegating to that
module may be the surest way to determine if a module is enabled based on
the flags? What do you think? What's the table optimizing for? If contrive
have no side effects, they should just be initialized, check if enabled and
if not, shouldn't have any references after the newenablednsqaddons returns

Copy link
Member

Choose a reason for hiding this comment

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

No, I don't think it's a strict requirement. Just that it makes some sense to put this code which filters the mod-opts in one place, instead of in each module.

This also enables better "invalid option" checking - if the common code ensures that every option is for a module it knows of, and every module ensures that it understands every option it gets.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good, will update

@ploxiln
Copy link
Member

ploxiln commented Jul 23, 2017

I just re-added a comment about this approach. But you should probably not actually work on it if @mreiferson is against this.

I am sorry if I've led you on a fruitless path, but in my defense, I've tried to be clear that I was only giving my opinion 😬

@mreiferson
Copy link
Member

Hah!

I guess my position is that, unless it's truly going to be an opt-in plugin model, then why jump through all the hoops with internal code to structure it like that? I guess the argument would be that adopting a flag API like what we've got here would allow us to migrate to a plugin model in the future?

@dm03514
Copy link
Author

dm03514 commented Jul 23, 2017 via email

@ploxiln
Copy link
Member

ploxiln commented Jul 24, 2017

I can see a path forward to plugins, from this model. nsqd would need to look in a directory (probably from a flag option) for plugin files named e.g. name.so. It would get a reference to the "New()" function and the "Start()" function, and call them similarly as this does.

The point of plugins is so that they can be compiled separately from nsqd, from code in separate repositories, and then used with the released build of nsqd (I think). This repo might itself include some nsqd plugins, built along with all the apps by make. This would be good as an example and ongoing validation of the plugin interface, and for this kind of "contrib-addon-but-popular" functionality. But the main point of the plugins would be to enable separate-repo separate-build nsqd addons.

I don't think "paid/priority" is a thing we're thinking about. Just about trying to enable people to do what they want, without "muddying up" the core code, or needed to get agreement from all core contributors. It's really just another way to organize code to optimize for maintenance and flexibility.

@dm03514 dm03514 changed the title [WIP] contrib: skeleton app structure contrib: skeleton app structure & Dogstatsd nsqd addon Jul 30, 2017
@dm03514
Copy link
Author

dm03514 commented Jul 31, 2017

@ploxiln Could you possibly take another look? I added the initializer table to only initialize addons if one of their options are passed.

contrib/nsqd.go Outdated
var hasOpt bool

initializers := map[string]initializer{
"dogstatsd": NewNSQDDogStatsd,
Copy link
Member

Choose a reason for hiding this comment

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

@mreiferson if we used https://golang.org/pkg/plugin/ that functionality would go roughly here. We would scan for plugin files in some dir, open them and try to get two or three functions with standard names out.

The fact that the dogstatsd module is in the same "package" as this module loader makes it not match that pattern exactly - it has a unique init function name - but it's pretty close.

@ploxiln
Copy link
Member

ploxiln commented Jul 31, 2017

I have a couple minor stylistic quibbles, but I like the structure. @mreiferson are you somewhat convinced yet ;)

contrib/nsqd.go Outdated

for _, opt := range contribOpts {
if strings.Contains(opt, k) {
hasOpt = true
Copy link
Member

Choose a reason for hiding this comment

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

This match could be more exact.

This loop could append to a new array of opts (instead of breaking out), and pass that new array to the initializer function. Then, if the module initializer does not recognize any of the opts, it can warn or error about that (because it would not have to handle any opts not intended for it).

@dm03514
Copy link
Author

dm03514 commented Aug 14, 2017

@ploxiln updated to filter valid options (and only pass those to addon initializer), and user more accurate option matching :)

@zouyee
Copy link
Contributor

zouyee commented Aug 19, 2017

nice work!

@ploxiln
Copy link
Member

ploxiln commented Aug 19, 2017

I'll take another look real soon, was just a bit busy at work ...

@dm03514
Copy link
Author

dm03514 commented Aug 19, 2017 via email

@ploxiln
Copy link
Member

ploxiln commented Aug 23, 2017

@mreiferson I think I can clean this up a bit, in terms of API, before the next release, and I think I can work on transitioning this to use go plugins (though they are still linux-only in go-1.9 I think). Thoughts?

@dm03514 can you squash these commits into one?

@ploxiln
Copy link
Member

ploxiln commented Aug 24, 2017

actually, everyone just wait for a few days, I'll open another PR (which includes and adds to this one)

@mreiferson
Copy link
Member

WAITING 😳

@dm03514
Copy link
Author

dm03514 commented Oct 10, 2017

Is there any work I can help with?

@ploxiln
Copy link
Member

ploxiln commented Oct 13, 2017

It's very reasonable to wonder what is going on with this PR. I happen to be on a vacation overseas, so you won't see any update from me before the end of this month. (For what it's worth, I was busy trying to wrap things up at work before the vacation.) So, I apologize for holding this up. I have not forgotten about it :)

@dm03514
Copy link
Author

dm03514 commented Oct 13, 2017

Thank you @ploxiln !

@ploxiln ploxiln mentioned this pull request Oct 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants