Fasthttp support!!

[savsgio]

#925

[cla-checker-service]

💚 CLA has been signed

[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

• Build Cause: axw commented: jenkins run the tests please

• Start Time: 2021-06-04T12:15:46.188+0000

• Duration: 30 min 55 sec

• Commit: a233546

Test stats 🧪

Test	Results
Failed	0
Passed	10738
Skipped	266
Total	11004

Trends 🧪

[axw]

Thanks for opening the PR @savsgio! I've taken a first pass, it's looking good. I'd like to simplify the API as much as possible (namely remove the Transaction wrapper type), remove unsafe code, and make sure it works well with the context.Context-based agent APIs.

[axw]

Thanks for the updates. Left a few more comments.

As a more general comment: I'd strongly prefer to remove all pooling and optimisations until there is a complete, correct, and well unit tested implementation. Then with benchmarks we can see the impact of adding pooling, and whether it's worth the additional complexity.

[savsgio]

Hi @axw

Could you review it again? Please.

Thanks.

[axw]

Thanks for the updates @savsgio! Almost there, just a few small comments.

If it's not absolutely required, I'd like to unexport the TxCloser type to keep the API surface area minimal. If possible it would be nice to reuse apmhttp.StartTransactionBody; if not possible then no worries, but then we'll need to flip the order of W3C/Elastic traceparent headers.

Apart from those comments, could you please run make scripts/Dockerfile-testing update-licenses fmt

[axw]

LGTM, thank you for your contribution and for bearing with me! Just need to wait for a green CI run now.

[axw]

jenkins run the tests please

[axw]

jenkins run the tests please

[axw]

@savsgio there's some lint issues (check-vanity-imports is maybe a little too strict)
Can you please fix add the vanity import comment to each file? It might be worth running make check and fixing any issues you find, to avoid the lengthy round-trip through CI.

[savsgio]

After run make check, theapmredigo is failing:

=== RUN   TestRequestContext
    integration_test.go:124: 
                Error Trace:    integration_test.go:124
                                                        integration_test.go:34
                Error:          Received unexpected error:
                                WRONGPASS invalid username-password pair
                Test:           TestRequestContext
--- FAIL: TestRequestContext (0.03s)
=== RUN   TestPipelineSendReceive
    integration_test.go:124: 
                Error Trace:    integration_test.go:124
                                                        integration_test.go:69
                Error:          Received unexpected error:
                                WRONGPASS invalid username-password pair
                Test:           TestPipelineSendReceive
--- FAIL: TestPipelineSendReceive (0.00s)
=== RUN   TestPipelinedTransaction
    integration_test.go:124: 
                Error Trace:    integration_test.go:124
                                                        integration_test.go:98
                Error:          Received unexpected error:
                                WRONGPASS invalid username-password pair
                Test:           TestPipelinedTransaction
--- FAIL: TestPipelinedTransaction (0.00s)
FAIL
FAIL    go.elastic.co/apm/module/apmredigo      0.262s
FAIL
make: *** [test] Error 1

[axw]

Do you have REDIS_URL set in your environment? that's used by the tests to detect that they're running inside an integration testing environment. Maybe there's a collision with your environment.

Anyway, I think you can ignore those errors.

[axw]

jenkins run the tests please

[savsgio]

Do you have REDIS_URL set in your environment? that's used by the tests to detect that they're running inside an integration testing environment. Maybe there's a collision with your environment.

Anyway, I think you can ignore those errors.

haha ok!

[axw]

OK, last one: can you please add a "go1.9" build tag to each of the source files apart from doc.go in apmfasthttp? The agent still supports Go 1.8 generally, but fasthttp doesn't appear to so we'll need to disable it there.

[savsgio]

OK, last one: can you please add a "go1.9" build tag to each of the source files apart from doc.go in apmfasthttp? The agent still supports Go 1.8 generally, but fasthttp doesn't appear to so we'll need to disable it there.

The minimum supportted version by fasthttp is go 1.12.

Done!

[axw]

jenkins run the tests please

[axw]

jenkins run the tests please

[axw]

Thanks again @savsgio! 🎉

[savsgio]

Thank you very much! @axw 😉 🎉