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

Fix context propagation across goroutines #118

Merged
merged 32 commits into from
Aug 7, 2023

Conversation

edeNFed
Copy link
Contributor

@edeNFed edeNFed commented Apr 27, 2023

What's changed

  1. Added 4 utility functions that should be used by instrumentors:
  • get_consistent_key: This function provides a key that works across any version of Go. This key should be used in the eBPF map created by the instrumentor to record the end timestamp at the end of the probed function.
  • start_tracking_span(void *contextContext, struct span_context *sc) - This function should be used in order to perform in-process context propagation, meaning passing context between different instrumentations in the same applications (see below test case for example).
  • stop_tracking_span(struct span_context *sc) - This function marks the passed span context as no more relevant for context propagation. This function should usually be called when recording the end timestamp of a span.
  • get_parent_span_context(void *ctx) - Use this function in order to get the parent span context if available. This function receives the address of context.Context object.
  1. Modified existing instrumentors to use those functions when reading/writing to eBPF maps and to performing context propagation between instrumentors.

  2. Modified gRPC (client) instrumentation to inject headers by probing loopyWriter.headerHandler instead of http2Client createHeaderFields: In Go >= 1.17, the return value is passed by registers and not by the stack. Currently, eBPF is not able to modify registers' values, only memory. Changing the uprobe to attach to loopyWriter.headerHandler allow us to add context-propagation headers even on Go versions above 1.17.

Validation

I used the following code as a test case:

func main() {
	http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
		con, err := grpc.Dial("grpc-server.default:50051", grpc.WithTransportCredentials(insecure.NewCredentials()))
		if err != nil {
			panic(err)
		}

		stopCh := make(chan struct{})
		go func() {
			healthClient := healthpb.NewHealthClient(con)
			resp, err := healthClient.Check(r.Context(), &healthpb.HealthCheckRequest{})
			if err != nil {
				panic(err)
			}

			fmt.Fprintf(w, "gRPC server status: %s", resp.Status)
			close(stopCh)
		}()

		<-stopCh
	})

	fmt.Println("HTTP server started on port 8080")
	err := http.ListenAndServe(":8080", nil)
	if err != nil {
		panic(err)
	}
}

Which produces the following trace (after the applied fix)
Screenshot 2023-05-22 at 1 11 33 PM

Tested on Go 1.15 and 1.20, both on arm64 and amd64

@edeNFed
Copy link
Contributor Author

edeNFed commented May 1, 2023

Blocked by: #82

@edeNFed edeNFed changed the title Refactor eBPF maps Change eBPF maps access functions May 2, 2023
@MrAlias
Copy link
Contributor

MrAlias commented May 2, 2023

Related to #86

@edeNFed edeNFed changed the title Change eBPF maps access functions Fix context propagation across goroutines May 19, 2023
@edeNFed edeNFed marked this pull request as ready for review May 22, 2023 14:10
@edeNFed edeNFed requested a review from a team as a code owner May 22, 2023 14:10
@edeNFed
Copy link
Contributor Author

edeNFed commented May 22, 2023

@open-telemetry/go-instrumentation-approvers this is ready for review, thanks! 🙏

Copy link
Member

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

This looks really good, thanks @edeNFed.

I've left some comments and suggestions.

It's worth noting #91 conflicts with this. Whichever PR lands first, will require the other to be rebased.

include/go_context.h Outdated Show resolved Hide resolved
include/go_context.h Outdated Show resolved Hide resolved
@damemi damemi mentioned this pull request Jul 11, 2023
@MikeGoldsmith
Copy link
Member

@edeNFed is this ready for another round of review or have you more changes planned?

@edeNFed
Copy link
Contributor Author

edeNFed commented Jul 23, 2023

@open-telemetry/go-instrumentation-approvers This is ready for another review
Thank you 🙏

Copy link
Member

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @edeNFed 👍🏻

@edeNFed edeNFed merged commit 09f5ad9 into open-telemetry:main Aug 7, 2023
11 checks passed
@MrAlias MrAlias added this to the v0.3.0-alpha milestone Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants