Preserve compressed agent data and compress agent data that is not already compressed #72

estolfo · 2021-11-08T13:26:05Z

This PR resolve #71

The changes include:

Create a struct for representing the agent data. It has members for the agent data and content encoding of the request
Change the type of the agent data channel to the AgentData struct
Don't decompress the agent data when it is received by the extension
When the extension sends the data to the apm server, compress data that is not already compressed and just pass along the content encoding and unchanged data otherwise.
Remove the getDecompressedBytesFromRequest function, as we won't need it anymore
Remove the tests for the getDecompressedBytesFromRequest function

…ready compressed

apm-lambda-extension/extension/apm_server.go

ghost · 2021-11-08T13:30:15Z

💚 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: 2021-11-15T09:38:30.100+0000
Duration: 5 min 22 sec
Commit: 2a93da7

Test stats 🧪

Test	Results
Failed	0
Passed	54
Skipped	0
Total	54

🤖 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!)

apm-lambda-extension/extension/apm_server.go

astorm

Looks like a great start and the general shape of things looks correct to me. Two things to address and (I think) we'll be set

I've not done it before but Felix's suggestion of using an io.Pipe() to avoid the compressedBytes buffer variable is sound
I have a question about error handling in the new code that reads the agent request body that might lead to a little extra work (or an explanation)

astorm · 2021-11-09T01:23:17Z

apm-lambda-extension/extension/route_handlers.go

+	if r.Body == nil {
+		log.Println("Could not get bytes from agent request body")
 	} else {
+		rawBytes, _ := ioutil.ReadAll(r.Body)


It looks like ioutil.ReadAll doesn't promise success. Should we be handling the error here instead of suppressing with an _? Or is that not needed?

I'll ask what the apm server does, as we should probably handle the error similarly to how they would.

https://github.com/elastic/apm-server/blob/master/beater/api/intake/handler.go#L72-L100

If the apm server can't read the request, it doesn't log the error but rather writes the error back to the client. I'm not sure we would want to write errors back to the agent, as the agent can't do anything more with the information than the extension can, and we don't want to hold up the lambda function. It seems to me like the best way to handle this would be to log the error and move on. @astorm what do you think? Maybe @felixbarny has some thoughts as well.

SGTM. If we find use cases that require sending a response to the agent, we can always add it later.

@estolfo logging the error and moving on seem sufficient to me as well.

estolfo · 2021-11-10T13:14:35Z

@felixbarny If the extension tries to compress non-compressed agent data and fails, what should the extension do? Send the data non-compressed?

felixbarny · 2021-11-10T13:31:00Z

The compression happens while sending data to APM Server. That means if an error happens, some data might have already been sent to the Server. I think the primary source of errors would be network issues rather than issues related to compressing the data. In these cases, I think it's fine to log the error and discard the data.

felixbarny · 2021-11-10T15:23:26Z

apm-lambda-extension/extension/apm_server.go

+			gw.Close()
+			pw.Close()
+			if err != nil {
+				log.Printf("Failed to compress data: %v", err)


[minor suggestion]
IIUC, this function will only be executed once the request has been established and is then streamed (via chunked-encoding) to the server.
If that's correct, the message may be a bit misleading as the more common source of failure is that there's a network issue when streaming the data to APM Server.

Suggested change

log.Printf("Failed to compress data: %v", err)

log.Printf("Failed to send compressed data: %v", err)

We are actually not streaming data yet, but rather sending in batches. At this point in the code, the actual failure is that the data could not be compressed in a go routine. If compression failed in that go routine, we will also get an error that is logged on line 52 (failed to create a new request when posting to APM server) and the function will return. So I think the error message here is accurate and should stay as-is.

I think this streams the batches. See also https://medium.com/stupid-gopher-tricks/streaming-data-in-go-without-buffering-3285ddd2a1e5#2342

Yes, you're right that the actual send could be chunked by the go internal transport code, I thought you were referring to the way that we send the data as it's received from the agent. But at this point in the code, I'm calling io.Copy, which ends up calling Write on the gzipWriter before it gets to the network code. So any compression failures would be returned by io.Copy. Network errors would be returned by resp, err := client.Do(req).

But that actually brings up another point: the error returned from client.Do(req) actually wraps the compression error, which means we don't really need to do anything with the error returned from io.Copy. In other words, we could removing the logging of the error and just rely on the error returned in this line

I'm calling io.Copy, which ends up calling Write on the gzipWriter before it gets to the network code.

What I'm understanding from the blog that I linked is that io.Copy blocks until the PipeReader is being read from. This happens when client.Do(req) is invoked and the data from the PipeReader is written into the HTTP request body via chunked encoding. It's because io.Copy blocks that we have to execute it in a concurrently running goroutine. If we didn't do that, we'd never get to the client.Do(req) part.

See also the docs for io.Pipe

each Write to the PipeWriter blocks until it has satisfied one or more Reads from the PipeReader that fully consume the written data. The data is copied directly from the Write to the corresponding Read (or Reads); there is no internal buffering.

astorm

Happy to approve once we have logging for the error case on

rawBytes, _ := ioutil.ReadAll(r.Body)

* Adding local lambdas and execution script * Change current transaction name instead of creating a new one Also added an env variable to toggle output, and modified the test flow to make sure that the right permissions are always set for the java agent layer * Parallelize Lambda execution * Replace Elasticsearch by a mock APM server * Cleanup env variable checks and go.mod * Change test name and write Lambda paths as variables * Make the Java APM Agent version an env. variable * Remove test files and fix folder detection * Improve request response decoding (Based on PR #72) * Refactor channel use to avoid test block by a single lambda * Make the tests single-language and fix Gradle * Set the default config values * Fix default values * Add timer defer and add units to doc * Add tolerance for Uppercase, but set the documented language value to lowercase. * Add supported languages in Panic message * Replace"node" by "nodejs" * Print the UUID * Variable/Function name refactor * Return empty string upon timeout and add new line to server log

Preserve compressed agent data and compress agent data that is not al…

4b97826

…ready compressed

github-actions bot added the aws-λ-extension AWS Lambda Extension label Nov 8, 2021

estolfo added 3 commits November 8, 2021 14:27

Change name and type of agent data channel

c2dc879

Accidentally removed a test

917742a

Use same body variable

f3f1bb6

estolfo commented Nov 8, 2021

View reviewed changes

apm-lambda-extension/extension/apm_server.go Outdated Show resolved Hide resolved

estolfo requested review from astorm and felixbarny November 8, 2021 13:31

felixbarny reviewed Nov 8, 2021

View reviewed changes

apm-lambda-extension/extension/apm_server.go Outdated Show resolved Hide resolved

apm-lambda-extension/extension/apm_server.go Outdated Show resolved Hide resolved

astorm reviewed Nov 9, 2021

View reviewed changes

Use BestSpeed compression when sending to APM server

d5d2715

Use io.Pipe instead of separate buffer when compressing data

aff9443

estolfo added 3 commits November 10, 2021 14:43

Log error if data cannot be compressed

c2e479d

Remove Todo note

4744109

Use err variable

55bf717

estolfo requested review from astorm and felixbarny November 10, 2021 13:51

Use ioutil.ReadAll instead for go 1.15

ea0ef6f

felixbarny approved these changes Nov 10, 2021

View reviewed changes

astorm reviewed Nov 10, 2021

View reviewed changes

Handle all possible errors when reading agent request body

2a93da7

estolfo merged commit 70c1c51 into main Nov 15, 2021

jlvoiseux added a commit to jlvoiseux/apm-aws-lambda that referenced this pull request Jan 31, 2022

Improve request response decoding (Based on PR elastic#72)

6f748fc

simitt deleted the estolfo/compression branch August 23, 2022 12:49

	log.Printf("Failed to compress data: %v", err)
	log.Printf("Failed to send compressed data: %v", err)

Preserve compressed agent data and compress agent data that is not already compressed #72

Preserve compressed agent data and compress agent data that is not already compressed #72

Uh oh!

Conversation

estolfo commented Nov 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ghost commented Nov 8, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

Uh oh!

Uh oh!

Uh oh!

astorm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estolfo commented Nov 10, 2021

Uh oh!

felixbarny commented Nov 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astorm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

estolfo commented Nov 8, 2021 •

edited

Loading

ghost commented Nov 8, 2021 •

edited by ghost

Loading