Introduce HandlerTrace callbacks

[willnewrelic]

Hi

Now that NewHandler is public, it is possible to write "middleware" instrumentation that wraps the handler. eg. lambda.Start(myHandler) becomes lambda.StartHandler(addMiddleware(myHandler)). Fantastic!

Unfortunately, since the internal lambdaHandler does the JSON serialization and deserialization, the middleware doesn't have access to the request and response events. This is unfortunate because middleware instrumentation would really love to record request and response fields (such as the request event source ARN).

Consumers could be asked to modify their handlers to add callbacks, but it sure would be nice if the middleware could access the request and response automatically!!

Thinking through this problem, I was reminded of the httptrace package which allows people to add http instrumentation by adding callbacks to the context. That's the idea presented here!

Example use:

func myHandler(ctx context.Context, x int) (int, error) {
	return 456, nil
}

type handlerFunc func(ctx context.Context, payload []byte) ([]byte, error)

func (h handlerFunc) Invoke(ctx context.Context, payload []byte) ([]byte, error) {
	return h(ctx, payload)
}

// introduceMiddleware would exist in a third party package
func introduceMiddleware(originalHandler interface{}) lambda.Handler {
	handler := lambda.NewHandler(originalHandler)
	return handlerFunc(func(ctx context.Context, payload []byte) ([]byte, error) {
		ctx = lambda.WithHandlerTrace(ctx, lambda.HandlerTrace{
			RequestEvent: func(c context.Context, e interface{}) {
				fmt.Println("request", e)
			},
			ResponseEvent: func(c context.Context, e interface{}) {
				fmt.Println("response", e)
			},
		})
		return handler.Invoke(ctx, payload)
	})
}

func main() {
	lambda.StartHandler(introduceMiddleware(myHandler))
}

Thanks for your consideration!

[codecov-io]

Codecov Report

Merging #141 into master will increase coverage by 0.91%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #141      +/-   ##
=========================================
+ Coverage   68.89%   69.8%   +0.91%     
=========================================
  Files          17      18       +1     
  Lines         659     679      +20     
=========================================
+ Hits          454     474      +20     
  Misses        165     165              
  Partials       40      40

Impacted Files	Coverage Δ
lambda/handler.go	97.1% <100%> (+0.22%)	⬆️
lambda/handlertrace/trace.go	100% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 527f5d3...f830414. Read the comment docs.

[willnewrelic]

Extra thoughts!

The context parameter to the callbacks might not be necessary (people can put state in anonymous functions), but it seems like a convenient thing to have.

I chose functions (over interface values) for the callbacks because they are easier to compose.

Note that if WithHandlerTrace is called multiple times, the extra callbacks will be added (rather than overwritten), which means that multiple middleware can make use of this!

[willnewrelic]

This PR has been rebased on the latest master. It is ready to go!

[bmoffatt]

Hey Will, apologies for the wait.

I've got 2 open concerns.

1. For documentation and auto-complete purposes, I'd like to keep the lambda module super lean. Moving the types to, ex: lambda/handler_trace, would keep the first interface everyone sees simple. This is what is done for lambdacontext.LambdaContext.
2. We still don't have a good middleware story for serialization/de-serialization, or for what the future of HandlerTrace would be for users who choose to use an alternative serde implementation. I'd prefer a unified middleware story to a split one, but that might not be possible with Go's limitations ☺️ Am interested in any creative ideas here, but not blocking to getting this merged.

Anyways, looks good, even tough it makes me wish for generics!

[willnewrelic]

Hi @bmoffatt

Great!!

I have rebased this PR and moved the new functions into a new handlertrace package. Ready for your review!

I don't know what the best middleware story would be, but I think this is a good forward step!

I expect that only middleware authors are going to be using lambda.NewHandler, so that could be moved into handlertrace or some other internal package. I have not done that to keep this PR focused and avoid breaking consumers already using lambda.NewHandler.

[bmoffatt]

👍 with renames

[bmoffatt]

rename as NewContext

[willnewrelic]

Done.

[bmoffatt]

rename as FromContext

[willnewrelic]

Done.

[willnewrelic]

@bmoffatt Functions renamed. Back to you!

[bmoffatt]

Thanks! This'll get included in a release tag soon as well.