Proposal: Publisher middleware and context

[purple4reina]

Hello, I would like to share a proposal with you all for adding middleware and context to publish calls. It's still got work left, but I wanted to stop here and get your thoughts and feedback.

Background

I work at New Relic where we help our customers monitor their application performance. Part of this is gathering timing information for messaging tools like NATS. Right now, in order to be able to do any timing of Publish calls, users must manually start and stop timers with every call. However, with the use of a middleware, they'd only need to write this code once and have it called every time. This is similar to Conn.Statistics but more fine grained.

There are lots of other uses for a middleware like this too. Conditionally adding headers or updating the data/subject. Logging each time a publish call is made. Sending errors to an error reporting system.

Implementation

This proposal includes two pieces of work. Though distinct, they work best when completed together. First is the ability to add the PublishWrapper as described above. Second is the need to pass a context.Context into this wrapper. This is important because the logic of your wrapper may depend on the context in which it is called. This is particular important for our New Relic agent because we must have access to the transaction context in which a publish call is made.

Open Questions

Adding context to all publish calls means that Go versions 1.6 and earlier will no longer be supported for this package. Since official support for Go v1.6 ended in Feb 2017 (over two and a half years ago), we were wondering if this would be an acceptable change. If not, how long were you planning on supporting Go v1.6? In that case, I’m open to ideas on how I could move the context.Context references I added to the context.go file. However, if this is a deprecation you’re okay with, I’d love your feedback on how to make this PR merge ready. What tests would you like to see? How could the documentation be improved?

Thanks a ton and looking forward to hearing back from you all!

[coveralls]

Coverage decreased (-0.5%) to 91.647% when pulling 38b8e360c775006b031d7f996af7f9b72329cb02 on purple4reina:publish-wrapper into 6ffcfbcbf08bc73e2158a0685edec6b32c941d38 on nats-io:master.

[derekcollison]

Really like the idea of middleware. The EncodedConn was a stop gap but what we really need is true middleware, so excited to take a look.

[purple4reina]

@derekcollison I spent a good amount of time looking into how users could register PublishWrappers by subject and found no perfect solution. Here's some options I came up with. Let me know your thoughts or if I've missed anything here. 😄

Problem: The proposal is that, given the subject of a publish call, apply only a subset of PublishWrappers on it based on the value of the subject. My instinct is to keep this routing as similar to the routing already done between Publishers and Subscribers. For example, a subject of foo.bar would apply the PublishWrappers for foo.bar and foo.*. However, all of this routing is currently handled by the NATS server, not here in the client, so this mapping of subjects to PublishWrappers would have to be added functionality.

1. Import routing code from server

In this option we could import code from the server repo to help with this routing.

Pros:

• Consistent routing logic between these PublishWrappers and the server
• Code written in only one place

Cons:

• Added dependency to this client package
• Publish calls are slower because routing logic must be applied

2. Copy and paste routing code from server

In this option we could copy what we need out of the server repo and add it here.

Pros:

• No new dependencies added

Cons:

• Routing logic is now kept in two locations
• Publish calls are slower because routing logic must be applied

3. Implement a simplified routing system

This could be something like requiring an exact match of the subject with the exception of ">" which would match all PublishWrappers.

Pros:

• Publish calls are faster because less logic is being applied
• No new dependencies added

Cons:

• The simplification of routing could be surprising and would require detailed documentation
• There would be no way for users to specify if they want their foo.bar wrapper to be before/after their > wrapper. They would be stored in different slices and therefore we would need to make the decision now which of these two slices to apply first.

4. Let users handle routing

In this option, we do nothing and leave the code as it is proposed here. Since the subject is passed as one of the args to the PublishFunc, the user can have access to it at call time and make a routing decision on their own.

Pros:

• Publish calls are only as slow as the user makes them
• No new dependencies added
• Users have total control

I like this last option the best. Here's an example of how a user could take advantage of it:

publishWrapper := func(fn nats.PublishFunc) nats.PublishFunc {
	return func(nc *nats.Conn, ctx context.Context, subj, reply string, data []byte) error {
		if strings.HasPrefix(subj, "foo.") {
			log.Printf("subject: %s, bytes: %d\n", subj, len(data))
		}
		return fn(nc, ctx, subj, reply, data)
	}
}
nc, _ := nats.Connect(nats.DefaultURL, nats.RegisterPublishWrappers(publishWrapper))

I'm totally open to other ideas though, so just let me know what you're thinking!

[kozlovic]

@purple4reina Thanks! Will get back to you asap.

[purple4reina]

Update: So it looks like I was wrong! Calling context.Background() does not in fact do an allocation. The underlying type of the returned context is just a pointer to an int. I did some benchmarks on my laptop of the time it takes to perform this call. It's about 0.5 ns.

This was confirmed to me by running go test -bench=BenchmarkPublishSpeed$ -run=^$ -benchtime=10s ./... on this branch as it is now (passing nil for the context when it is not needed) and as it was in the prior commit (passing in a new context.Background() for every publish). I saw very similar timings for each:

# using `nil`
goos: darwin
goarch: amd64
pkg: github.com/nats-io/nats.go/test
BenchmarkPublishSpeed-8         80679073               148 ns/op
PASS
ok      github.com/nats-io/nats.go/test 21.432s

# using `context.Background()`
goos: darwin
goarch: amd64
pkg: github.com/nats-io/nats.go/test
BenchmarkPublishSpeed-8         80696215               146 ns/op
PASS
ok      github.com/nats-io/nats.go/test 21.550s

# master branch
goos: darwin
goarch: amd64
pkg: github.com/nats-io/nats.go/test
BenchmarkPublishSpeed-8         72735517               138 ns/op
PASS
ok      github.com/nats-io/nats.go/test 10.334s

This leads me to suspect that the 10%+ increase in overhead we're seeing is not from the call to context.Background() but rather from the wrapping of the publish calls each time.

I'll continue investigating tomorrow and get back to you with a better solution.

[purple4reina]

@kozlovic happy Friday! I have uncovered the source of our overhead! It ends up it had nothing to do with the addition of the context. It was that I had added additional function calls into the stack and each of those was taking about 5ns of additional time. Removing those calls puts latency back closer to where it is on the master branch.

# this branch
$ go test -bench=BenchmarkPublishSpeed$ -run=^$ -benchtime=10s -benchmem ./...
goos: darwin           
goarch: amd64          
pkg: github.com/nats-io/nats.go/test          
BenchmarkPublishSpeed-8         68088583               161 ns/op               4 B/op          0 allocs/op

# master branch
$ go test -bench=BenchmarkPublishSpeed$ -run=^$ -benchtime=10s -benchmem ./...
goos: darwin
goarch: amd64
pkg: github.com/nats-io/nats.go/test
BenchmarkPublishSpeed-8         69659853               155 ns/op               3 B/op          0 allocs/op

The main change I made to accomplish this is to wrap the publish method early and only once instead of with each call. This has the consequence of locking in the PublishWrappers option once Connect has been called which goes against convention for the other options which can all be amended after Connect.

Let me know your thoughts on this change. Again, I appreciate your time and willingness to look at this PR.

[derekcollison]

Can you squash all these down to one commit for easier review? Thanks!

[purple4reina]

Hey @derekcollison and @kozlovic! Any word on this pull request? Thanks again for considering it. Just wondering if there's anything I can do to help make review easier and faster on your end.

[derekcollison]

Apologies for not circling back. In general we like the concept but want to have some time to discuss with the team. We have some other plans for the clients that we would want this to fold into nicely. Hopefully you can continue to run with your code as a layer above and we are not blocking you. We will get to this eventually, but involves a bunch of additional moving pieces that the team is considering.

[derekcollison]

We are starting to look into this a bit more now with the work on generic headers.

[derekcollison]

Have you looked at https://github.com/nats-io/not.go for an example for your implementation? Seems similar.