go-nsq icon indicating copy to clipboard operation
go-nsq copied to clipboard

Allow decorating ConsumerDelegate

Open colega opened this issue 5 years ago • 3 comments

We want to instrument our NSQ clients and detect the E_FIN_FAILED errors received here:

https://github.com/nsqio/go-nsq/blob/61f49c096d0d767be61e4de06f724e1cb5fd85d7/conn.go#L530-L532

It looks like the best way of doing this would be to implement a custom ConnDelegate listening to the OnError method:

https://github.com/nsqio/go-nsq/blob/61f49c096d0d767be61e4de06f724e1cb5fd85d7/delegates.go#L66-L68

Instead of just calling consumerConnDelegate

https://github.com/nsqio/go-nsq/blob/61f49c096d0d767be61e4de06f724e1cb5fd85d7/delegates.go#L111

Which actually does nothing

https://github.com/nsqio/go-nsq/blob/61f49c096d0d767be61e4de06f724e1cb5fd85d7/consumer.go#L680

Maybe instead of using just one delegate, we could have a slice of ConnDelegates in the consumer? Since those are not exported, that would not change the API and will not affect the existing users.

In order to keep backwards compatibility, we'd have to create a

func NewConsumerDelegates(topic string, channel string, config *Config, delegates ...ConnDelegate) (*Consumer, error) {

Constructor, or even better, looking into the future where someone else may need configuring something else, the constructor may accept functional options with something like:

type ConsumerOption func(*Consumer)

func WithConsumerConnDelegate(delegate ConnDelegate) ConsumerOption {
	return func(r *Consumer) {
		r.delegates = append(r.delegates, delegate)
	}
}

func NewConsumerWithOptions(topic, channel string, config *Config, options ...ConsumerOption) (*Consumer, error) {
	// ...
	r := &Consumer{ /* ... */}
	for _, applyOption := range options {
		applyOption(r)
	}
	// ...
	return r, nil
}

Of course, another option would be adding a ConnDelegate slices into the existing Config, but I'm not sure how open is that for accepting behaviours instead of just values.

colega avatar May 09 '19 15:05 colega

@ploxiln can I ask for you feedback on this as you're the most recently active member of NSQ?

colega avatar Sep 23 '19 15:09 colega

Ping @mreiferson @jehiah 🙏 We would be happy to contribute back this patch but would appreciate some early feedback 😃

jvrplmlmn avatar Oct 23 '19 07:10 jvrplmlmn

Of course, another option would be adding a ConnDelegate slices into the existing Config, but I'm not sure how open is that for accepting behaviours instead of just values.

I'm curious at what this implementation would look like, as I'd prefer to not have the "how should we set options" debate and instead more specifically think through the tradeoffs of making it possible for users to chain delegates.

mreiferson avatar Jun 14 '20 03:06 mreiferson