Restructure Packages and remove State Variables

[jlvoiseux]

Motivation / Summary

This PR aims to resolve several tech-debt-related issues, as well as to integrate improvements discussed during past code reviews.

Changes

• Resolve Investigate using a context passed to the logs API subscribe function #143
• Resolve Move the APM Data processing goroutine to apm_server.go #151
• Resolve Replace package-level variable by main-instantiated structs #178
• Move logger to the main package and replace function logging by error processing Out of scope, relevant issue to be created (Restructure files / packages to move towards a flatter structure #192)

How to test

• Switch to the related branch, create and upload the extension as a layer and use it to instrument an empty Lambda function.
• Alternatively, the following repo can be used with the corresponding release.

[apmmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Start Time: 2022-05-19T09:55:36.232+0000

• Duration: 7 min 4 sec

Test stats 🧪

Test	Results
Failed	0
Passed	200
Skipped	4
Total	204

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

• /test : Re-trigger the build.

• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[axw]

Here's some initial feedback.

Would it be possible to break this up a little bit? Maybe even just splitting 393aeef out into a separate PR? It's difficult to provide a comprehensive review when there are unrelated changes.

[jlvoiseux]

@axw Thank you for your feedback! I have rebased 393aeef - that one will be the object of a later PR.

[jlvoiseux]

There might be a way to remove one of these two calls, in a similar fashion to what we did with logsapi.Subscribe and logsTransport.

[axw]

Thanks @jlvoiseux, looking good.

I'm a bit concerned about races between agents requesting a flush, the completion of the invocation according to runtimeDone, and the flush timer. As it is we could mistakenly think we should flush earlier than we should. I'd like to dig into the requirements a bit here to see if we can simplify things.

[axw]

I'm a bit concerned about races between agents requesting a flush, the completion of the invocation according to runtimeDone, and the flush timer. As it is we could mistakenly think we should flush earlier than we should. I'd like to dig into the requirements a bit here to see if we can simplify things.

After some offline discussions, it seems like what we have is desirable. It would be good to document some of the reasoning in the code (maybe at the select loop in processEvent) for posterity.

Still, we need to ensure signals cannot leak between invocations.

[jlvoiseux]

@axw, @felixbarny, thank you for your reviews. I have implemented the related corrections last Friday.

To solve the race related to AgentDoneSignal, I have reproduced it by adding a test in main_test.go. This test simulates the case where the AgentDoneSignal corresponding to the invocation i is received after the RuntimeDoneSignal, thus interrupting the execution of the invocation i+1.

This scenario is avoided by opening and closing the AgentDoneSignal for each Lambda invocation. A limitation of this solution is that the edge case where the AgentDoneSignal of the invocation i is received during the invocation i+1 - but the only surefire way to handle this case would be to decompress the APM Payload to check faas.execution_id. Moreover, this scenario happening is very unlikely, as the Agent will have been stopped and restarted by the Lambda service by the time we reach the next invocation.

[axw]

Thanks for the updates @jlvoiseux. Although there's still a theoretical race with AgentDoneSignal, it's at least no worse than it was before. Maybe we can eliminate it in the future.

[jlvoiseux]

Logs API Issue Fix
The issue mentioned offline was due to me overlooking the following documentation item:

HTTP (recommended) – Lambda delivers logs to a local HTTP endpoint (http://sandbox.localdomain:${PORT}/${PATH}) as an array of records in JSON format. The $PATH parameter is optional. Note that only HTTP is supported, not HTTPS. You can choose to receive logs through PUT or POST.

Prior to the PR, the default listener address was set as a global variable to sandbox and then modified to accommodate our tests needs. This is no longer the case thanks to the implementation of InitLogsTransport(). However, the default listener address set by that function was localhost - the value required by our tests. To solve this issue:

• 84b261a: A Lambda environment detection mechanism has been implemented by using reserved environment variables
• 4c3e476: Corresponding unit tests have been added

Benchmark
The numerous changes implemented by the current PR did not impact performance or error rate in high throughput scenarios:

v1.0.0

Current PR

[axw]

Thanks for the update! Change looks good.