HTTP client spans #87
Conversation
|
Hey @mariomac, I'm open to splitting this into multiple PRs, let me know if it's unreasonable to review. |
Codecov Report
@@ Coverage Diff @@
## main #87 +/- ##
==========================================
+ Coverage 73.07% 73.95% +0.87%
==========================================
Files 19 20 +1
Lines 1315 1463 +148
==========================================
+ Hits 961 1082 +121
- Misses 264 289 +25
- Partials 90 92 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
bpf/go_common.h
Outdated
| __type(key, void *); // key: pointer to the request goroutine | ||
| __type(value, func_invocation); | ||
| __uint(max_entries, MAX_CONCURRENT_REQUESTS); | ||
| __uint(pinning, LIBBPF_PIN_BY_NAME); |
bpf/go_common.h
Outdated
| __uint(max_entries, MAX_CONCURRENT_REQUESTS); | ||
| __uint(pinning, LIBBPF_PIN_BY_NAME); |
There was a problem hiding this comment.
Maybe after this PR is merged, we should also evaluate whether we will have any type of conflict/corruption if we use the same BPF map from multiple auto-instrumenters in the same host.
If that happened, maybe we can try to assign a unique per-process map name on load so they are not shared (it seems Cilium library would allow that).
Another option is to override the /sys/fs/bpf path a unique /var/run/otelauto/... subfolder. LibBPF allows doing it but I don't know if Cilium lib can.
There was a problem hiding this comment.
Great point, I think we should privatise these paths based on the otelauto PID, as well as you suggest adding otelauto as prefix. I don't think it would be a problem to mount another path, we should be good to go with any as long as the mount type is bpf, which we already have to do for systems that don't have the bpf file system auto-mounted.
cmd/otelauto/main.go
Outdated
| @@ -33,6 +38,14 @@ func main() { | |||
| os.Exit(-1) | |||
| } | |||
|
|
|||
| c := make(chan os.Signal, 1) | |||
There was a problem hiding this comment.
To be deleted, as the last version in the main branch already implements graceful desktop.
| func erasePinnedMaps() { | ||
| for _, m := range fs.PinnedMaps { | ||
| slog.Debug("cleaning storage used by pinned object", "map", m) | ||
| err := os.Remove(filepath.Join(fs.PinnedRoot, m)) | ||
| if err != nil { | ||
| slog.Error("can't remove pinned map "+m, err) | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Related to a previous comment: we should be careful with this. If there are other otelauto instances running in the host, will they have their runtime data erased?
| sp.End(trace2.WithTimestamp(spans[i].End)) | ||
| span := &spans[i] | ||
|
|
||
| if span.Type == transform.EventTypeHTTPClient { |
There was a problem hiding this comment.
since gocyclo threshold has been decreased to 10, maybe it complains about this method. In that case I'd suggest to split some blocks into methods/functions.
|
Thanks for the thorough review Mario, I'll resolve the conflicts, merge and then work on the issues related to erasing conflicting maps. |
|
I'll follow-up with a PR to resolve the path sharing. |
This PR re-introduces the HTTP client spans. It does so by tapping into http.client send and then it tracks the goroutine creation to match with a parent HTTP/GRPC span.
The PR might be too large to review, so I think I'll start splitting it off into smaller PRs. I had to do some ground work to make this happen: