Fix net/http instrumentation for register-based ABI #34
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
edeNFed
approved these changes
Mar 9, 2023
|
lgtm |
MrAlias
reviewed
Mar 9, 2023
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
MrAlias
approved these changes
Mar 27, 2023
|
@mariomac would this change be relevant to the gorillamux instrumentor too? |
This was referenced Apr 18, 2023
JamieDanielson
added a commit
to honeycombio/opentelemetry-go-instrumentation
that referenced
this pull request
Apr 20, 2023
MrAlias
added a commit
that referenced
this pull request
May 12, 2023
* adjust output for e2e test to remove empty span * only use one http function in http server instr * Revert "only use one http function in http server instr" This reverts commit c1bc4d0. * only use one http function - keep refactor * keep both func names but only register 1 probe * mirror changes to gorillamux from server #34 * no more empty spans for gorillamux * revert http funcname to 1, keep range for register * make generate * fix error logger, merge some main changes * update changelog * Update CHANGELOG.md Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Update CHANGELOG.md Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * Update pkg/instrumentors/bpf/github.com/gorilla/mux/bpf/probe.bpf.c Co-authored-by: Mike Goldsmith <goldsmith.mike@gmail.com> * set Method size to 7 in gorillamux probe * set Method size to 7 in gin-gonic probe * set Method size to 7 in nethttp probe * updated traces.json after span name pr merged * also set method len to 7 in gorillamux probe c * also set method len to 7 in gin-gonic probe c * also set method len to 7 in nethttp probe c * make docker-generate --------- Co-authored-by: Mike Goldsmith <goldsmith.mike@gmail.com> Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Makes the stdlib net/http instrumentor to work on register-based ABIs.
Previously, it used the request context pointer as a key for the requests map. With register-based ABI, you can get the arguments in the start uprobe but not in the return uprobes, because the CPU registers might have changed at this point of the execution.
This PR changes the pointer to the context by a pointer to the actual Goroutine that executes the handle, which is guaranteed to be always in the CPU register 14 according to the Go ABI specification.
I have also observed that, at least in recent versions of Go, eBPF probes attached to
net/http.(*ServeMux).ServeHTTPare not triggered, so I added a newnet/http.HandlerFunc.ServeHTTPfunction reference and make sure that hooks are added to all the functions.