High-level code review before GA

[estolfo]

Before we GA the aws lambda extension, we should have an experienced Go engineer from another team (preferably the apm server team) review the codebase.

[simitt]

@elastic/apm-server team please conduct a high level review. The goal is to agree on a GA ready state of the code.
I suggest we put some focus points on the review:

• are errors properly handled
• any kind of deadlocks or unexpected blocking on channels; are channels closed while still consumed, etc.
• handling of incoming data
• handling of outgoing requests to apm-server
• general data structs (allocations, references, ..)
• e2e test coverage
• unit test coverage

This is not expected to be included as go module in other programs, therefore reviewing public interfaces etc. has less priority. The cli is written in nodejs, therefore please ignore this part in the golang review.

[estolfo]

If someone needs an overview of the code structure before doing a review, please let us know.

[axw]

I've just started looking in the logsapi package, and how it is used, and here are some initial thoughts:

If you're not familiar with it already, I'd suggest having a look through https://github.com/golang/go/wiki/CodeReviewComments. This is followed in many Go projects, to the point that some linters have checks for the rules. You might have a bad time if you want to turn on linting later on.

Some more detailed code docs would be great. As I'm not familiar Lambda internals, or the Logs API, it wasn't clear to me what the HTTP listener was for. Linking to the relevant parts of https://docs.aws.amazon.com/lambda/latest/dg/runtimes-logs-api.html may be helpful to future readers.

Does everything that is exported need to be exported? Having a minimal public interface helps clarify which parts are implementation details, and which parts are relevant to consumers. e.g. should users of this package care about the different protocols? I realise @simitt said "This is not expected to be included as go module in other programs, therefore reviewing public interfaces etc. has less priority.", but I still think it's important for future maintainers.

The Logs API docs state:

We also recommend setting up the local HTTP listener or the TCP port before subscribing to receive logs.

It appears that this is not being done. In main.go, subscription is happening first, and then a listener is created and finally started:

apm-aws-lambda/apm-lambda-extension/main.go

Lines 83 to 98 in b010fd9

	err = logsapi.Subscribe(
	extensionClient.ExtensionID,
	[]logsapi.EventType{logsapi.Platform})
	if err != nil {
	log.Printf("Could not subscribe to the logs API.")
	} else {
	logsAPIListener, err := logsapi.NewLogsAPIHttpListener(logsChannel)
	if err != nil {
	log.Printf("Error while creating Logs API listener: %v", err)
	}
	// Start the logs HTTP server
	_, err = logsAPIListener.Start(logsapi.ListenOnAddress())
	if err != nil {
	log.Printf("Error while starting Logs API listener: %v", err)
	}

Is there any reason this couldn't be abstracted away? Something like:

package logsapi

// Subscribe subscribes to the Lambda Logs API, sending log events to out until ctx is cancelled.
func Subscribe(ctx context.Context, extensionID string, eventTypes []EventType, out chan<- LogEvent) error

Internally, Subscribe would create an HTTP server (before making the subscribe request) and run it until ctx is cancelled. You might also want to listen on an ephemeral port to avoid any surprises with running multiple subscribers: listen on "sandbox.localdomain:0", and extract the ephemeral port from the net.Listener.Addr().

In main.go, there can be a panic if the "runtimeDoneSignal" channel is closed (e.g. due to "AgentDoneSignal" being signalled), and then a RuntimeDone log event is processed:

apm-aws-lambda/apm-lambda-extension/main.go

Line 170 in b010fd9

runtimeDoneSignal <- struct{}{}

apm-aws-lambda/apm-lambda-extension/main.go

Line 204 in b010fd9

close(runtimeDoneSignal)

Do you need to explicitly close that channel? It looks like you could just allow it to be garbage collected.

[estolfo]

Hi @axw
Thanks for your initial comments. It might be worth mentioning that the logsapi package code was actually taken from an aws example and we were planning on removing it when the agents implement this elastic/apm#569
Nevertheless, your suggestions make sense and we'll improve it as we might end up keeping the logsapi code to collect metrics.

In main.go, there can be a panic if the "runtimeDoneSignal" channel is closed (e.g. due to "AgentDoneSignal" being signalled), and then a RuntimeDone log event is processed:

Thanks for pointing that out, we'll look at that.

Do you need to explicitly close that channel? It looks like you could just allow it to be garbage collected.

Is it ok to not explicitly close a channel and just allow it to be garbage collected? I thought it was good practice to close a channel?

[axw]

Is it ok to not explicitly close a channel and just allow it to be garbage collected? I thought it was good practice to close a channel?

As a general rule, it's OK to not explicitly close it.

[axw]

apm-aws-lambda/apm-lambda-extension/extension/apm_server.go

Lines 39 to 59 in 86a442c

	buf := bufferPool.Get().(*bytes.Buffer)
	defer func() {
	buf.Reset()
	bufferPool.Put(buf)
	}()
	if agentData.ContentEncoding == "" {
	encoding = "gzip"
	gw, err := gzip.NewWriterLevel(buf, gzip.BestSpeed)
	if err != nil {
	return err
	}
	if _, err := gw.Write(agentData.Data); err != nil {
	log.Printf("Failed to compress data: %v", err)
	}
	if err := gw.Close(); err != nil {
	log.Printf("Failed write compressed data to buffer: %v", err)
	}
	} else {
	buf.Write(agentData.Data)
	}

Should those log.Printf be return err? If the compression fails, what's in buf?

Also, you could avoid some overhead in the case where ContentEncoding != "" by doing something like this:

var r io.Reader
if agentData.ContentEncoding != "" {
    r = bytes.NewReader(agentData.Data)
} else {
    encoding = "gzip"
    buf := bufferPool.Get().(*bytes.Buffer)
    defer func() {
        buf.Reset()
        bufferPool.Put(buf)
    }()     
    gw, err := gzip.NewWriterLevel(buf, gzip.BestSpeed)
    if err != nil {
        return err
    }       
    if _, err := gw.Write(agentData.Data); err != nil {
        log.Printf("Failed to compress data: %v", err)
    }
    if err := gw.Close(); err != nil {
        log.Printf("Failed write compressed data to buffer: %v", err)
    }
    r = buf
}

[axw]

apm-aws-lambda/apm-lambda-extension/extension/client.go

Lines 108 to 111 in 86a442c

	if httpRes.StatusCode != 200 {
	return nil, fmt.Errorf("extension register request failed with status %s", httpRes.Status)
	}
	defer httpRes.Body.Close()

You should move the defer before the status code check. Always close the response body if Do returns a nil error. Same applies in other methods in this file. A lot of the code is repeated: maybe define a helper function which sends a request to a specified path, and decodes the body into a given interface{}?

[axw]

apm-aws-lambda/apm-lambda-extension/extension/route_handlers.go

Lines 41 to 50 in 86a442c

	for name, values := range r.Header {
	// Loop over all values for the name.
	for _, value := range values {
	req.Header.Set(name, value)
	}
	}
	if err != nil {
	log.Printf("could not create request object for %s:%s: %v", r.Method, apmServerUrl, err)
	return
	}

Check error first, otherwise the req.Header.Set calls will panic if there's an error as req will be nil. Also, if there's an error, should the handler return a 5xx error status code?

apm-aws-lambda/apm-lambda-extension/extension/route_handlers.go

Line 53 in 86a442c

serverResp, err := client.Do(req)

The response body isn't being closed.

apm-aws-lambda/apm-lambda-extension/extension/route_handlers.go

Lines 89 to 90 in 86a442c

	rawBytes, err := ioutil.ReadAll(r.Body)
	defer r.Body.Close()

Would it be possible to stream the client's request body to apm-server, rather than reading it into memory?

I'm also wondering if this approach of using channels is necessary, or if the extension could be simplified by using https://pkg.go.dev/net/http/httputil#ReverseProxy? You could set a Director to do compression and tracking of in-flight requests so you can flush.

[felixbarny]

Is it ok to not explicitly close a channel and just allow it to be garbage collected? I thought it was good practice to close a channel?

As a general rule, it's OK to not explicitly close it.

That sounds similar to the deprecated Java finalizers that can close resources on GC. If it does work in a similar way, is it really a best practice?

[axw]

Go also has https://pkg.go.dev/runtime#SetFinalizer. Using this tends to be discouraged. I wouldn't say they're the same.

Channels don't actively consume any resources except their memory. Once they're no longer referenced, that memory can be garbage collected. If something were waiting for the channel to be closed, it couldn't be garbage collected. So it depends on the concurrency patterns, but in this case there's nothing using the channel's closure as an event.

[estolfo]

• Turn on linter Add linter to the codebase #153
• Some more detailed code docs Add more detailed code docs #154
• Set up the local HTTP listener or the TCP port before subscribing to receive logs (Refactor logsapi package #129)
• Abstract logsapi Subscribe function (Refactor logsapi package #129)
• listen on an ephemeral port to avoid any surprises with running multiple subscribers: listen on "sandbox.localdomain:0", and extract the ephemeral port from the net.Listener.Addr() (Refactor logsapi package #129)
• just allow runtimeDoneSignal channel to be garbage collected in main.go (Address partial list of code review comments #132)
• Should those log.Printf be return err? If the compression fails, what's in buf? in apm_server.go (Address partial list of code review comments #132)
• avoid some overhead in the case where ContentEncoding != "" in apm_server.go (Address partial list of code review comments #132)
• client.go You should move the defer before the status code check. Always close the response body if Do returns a nil error. Same applies in other methods in this file. (Address partial list of code review comments #132)
• client.go A lot of the code is repeated: maybe define a helper function which sends a request to a specified path, and decodes the body into a given interface{}?
• route_handlers.go Check error first, otherwise the req.Header.Set calls will panic if there's an error as req will be nil. (Address partial list of code review comments #132)
• If there's an intake error, should the handler return a 5xx error status code? (Address partial list of code review comments #132)
• route_handlers.go The response body isn't being closed. Line 53 (Address partial list of code review comments #132)
• Would it be possible to stream the client's request body to apm-server, rather than reading it into memory? (This isn't the right approach for the intake requests, but it does make sense for the info requests Use a reverse proxy for the info request to the apm server #138)
• I'm also wondering if this approach of using channels is necessary, or if the extension could be simplified by using https://pkg.go.dev/net/http/httputil#ReverseProxy? You could set a Director to do compression and tracking of in-flight requests so you can flush. (This is probably not the right approach given the goals of the extension)