Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[apmfasthttp] fix: set response context with closer close #1193

Merged
merged 14 commits into from Feb 21, 2022
Merged

[apmfasthttp] fix: set response context with closer close #1193

merged 14 commits into from Feb 21, 2022

Conversation

savsgio
Copy link
Contributor

@savsgio savsgio commented Jan 13, 2022

Fix and set correctly the response context data to the apm transacction after the request finish (included Hijack and Stream connections).

Relationed issue: #1104
Fasthttp issues: valyala/fasthttp#1176, valyala/fasthttp#1199

@elastic-apm-tech elastic-apm-tech added this to In Progress in APM-Agents (OLD) Jan 13, 2022
@apmmachine
Copy link
Collaborator

apmmachine commented Jan 13, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-02-21T01:22:05.713+0000

  • Duration: 44 min 45 sec

Test stats 🧪

Test Results
Failed 0
Passed 7356
Skipped 174
Total 7530

🤖 GitHub comments

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

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark test.

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

@stuartnelson3
Copy link
Contributor

Can you add a test to verify the fixed behavior?

@savsgio
Copy link
Contributor Author

savsgio commented Jan 13, 2022

Can you add a test to verify the fixed behavior?

The test added in #1104 tests the same case!

https://github.com/elastic/apm-agent-go/blob/master/module/apmfasthttp/server_test.go#L36

@axw
Copy link
Member

axw commented Jan 14, 2022

@savsgio the test you reference was succeeding without your change. Please add a test which would fail without your change, and succeeds with your change.

@savsgio savsgio changed the title [apmfasthttp] fix: set response context with closer close WIP: [apmfasthttp] fix: set response context with closer close Jan 18, 2022
@savsgio savsgio changed the title WIP: [apmfasthttp] fix: set response context with closer close [apmfasthttp] fix: set response context with closer close Jan 27, 2022
@savsgio
Copy link
Contributor Author

savsgio commented Jan 27, 2022

@axw could you review it again?, please!

@axw
Copy link
Member

axw commented Feb 1, 2022

Thanks @savsgio, I will take a look soon. We have a few other things going on too, so it may take a little while.

Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes! Looks good apart from the couple of comments I've left.

module/apmfasthttp/context.go Outdated Show resolved Hide resolved
module/apmfasthttp/server_test.go Outdated Show resolved Hide resolved
@savsgio
Copy link
Contributor Author

savsgio commented Feb 8, 2022

@axw Could you review it again? please

Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, LGTM. Thanks!

@axw
Copy link
Member

axw commented Feb 9, 2022

/test

@axw
Copy link
Member

axw commented Feb 9, 2022

@savsgio CI is unhappy:

17:47:36  + make install precheck check-modules
17:47:36  go get -v -t ./...
17:47:36  goimports differs:
17:47:36   - module/apmfasthttp/closer.go
17:47:36  Makefile:12: recipe for target 'check-goimports' failed

Please run make fmt to fix this issue, and make check to ensure there are no others.

@axw
Copy link
Member

axw commented Feb 17, 2022

@savsgio the change has broken the apmfiber tests. Can you please take a look at those?

@axw
Copy link
Member

axw commented Feb 21, 2022

/run elasticsearch-ci/docs

@axw axw merged commit 0c89cde into elastic:main Feb 21, 2022
APM-Agents (OLD) automation moved this from In Progress to Done Feb 21, 2022
@axw
Copy link
Member

axw commented Feb 21, 2022

Thanks @savsgio!

@savsgio
Copy link
Contributor Author

savsgio commented Feb 21, 2022

Thank you so much @axw!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants