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

[feat] Adding context propagation to net/http server instrumentation #92

Merged
merged 13 commits into from
May 23, 2023

Conversation

oxeye-gal
Copy link
Contributor

Hi,

This PR attempts to add context propagation to the net/http server instrumentation.

We do it by iterating over the buckets of the incoming request header map and trying to find the "traceparent" header.

This was tested with Go versions 1.12-1.19.

Our test environment consisted of instrumented Python application that sends a request to instrumented Go http server.

The produced trace looks like the following:

213373358-c184e90f-597c-4ca0-ba3f-52365fc89770

We would love to get your feedback on this.
Thanks!

@oxeye-gal oxeye-gal requested a review from a team April 24, 2023 07:04
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 24, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

damemi
damemi previously requested changes Apr 24, 2023
Copy link
Contributor

@damemi damemi 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 like you need to run make generate based on the failure

Also the e2e failure is interesting, because it seems that this change removed the redundant empty span that @JamieDanielson was trying to fix in #86 (see test output)

@oxeye-gal
Copy link
Contributor Author

@damemi I updated the bpf2go output for the server instrumentation, also the redundant empty span removed is due to the removal of the net/http.(*ServeMux).ServeHTTP symbol from the functions to instrument as also mentioned by #86

@damemi
Copy link
Contributor

damemi commented Apr 27, 2023

also the redundant empty span removed is due to the removal of the net/http.(*ServeMux).ServeHTTP symbol from the functions to instrument as also mentioned by #86

Ah I see. Is that necessary for context propagation to work in this change?

@JamieDanielson
Copy link
Member

also the redundant empty span removed is due to the removal of the net/http.(*ServeMux).ServeHTTP symbol from the functions to instrument as also mentioned by #86

Ah I see. Is that necessary for context propagation to work in this change?

Is it cleaner to keep both function names in this PR so the output of the test remains the same, and I can update the other PR after this is merged in? That way we separate concerns, because I don't think the extra empty span has an impact on this one (though I could be mistaken, I've been working on a few different branches of branches lately).

@damemi
Copy link
Contributor

damemi commented Apr 27, 2023

Is it cleaner to keep both function names in this PR so the output of the test remains the same, and I can update the other PR after this is merged in? That way we separate concerns

Yeah that's what I was wondering. if it's relevant to this PR, we can keep it, but otherwise @oxeye-gal can you remove that change so we don't duplicate the work?

@MikeGoldsmith
Copy link
Member

MikeGoldsmith commented May 3, 2023

Hey @oxeye-gal - this PR needs rebasing after we merged ARM support in #82. I’m happy to help do that if you would like us to.

@oxeye-gal oxeye-gal force-pushed the http_server_context_propagation branch from 0362e93 to 4471eb2 Compare May 6, 2023 22:40
@oxeye-gal oxeye-gal requested a review from damemi May 6, 2023 22:52
…trumentation into http_server_context_propagation

# Conflicts:
#	pkg/instrumentors/bpf/net/http/server/probe.go
@MikeGoldsmith MikeGoldsmith changed the title Adding context propagation to net/http server instrumentation [featAdding context propagation to net/http server instrumentation May 10, 2023
@MikeGoldsmith MikeGoldsmith changed the title [featAdding context propagation to net/http server instrumentation [feat] Adding context propagation to net/http server instrumentation May 10, 2023
@MikeGoldsmith
Copy link
Member

@oxeye-gal I've created a PR against your branch to fix the build.

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 for the contribution @oxeye-gal. I'll look at updating #91 next 👍🏻

@MikeGoldsmith MikeGoldsmith dismissed damemi’s stale review May 10, 2023 11:34

generated files have been updated, please re-review

@oxeye-gal
Copy link
Contributor Author

@MikeGoldsmith seems like the build fails again due to the added Request.Header offset, what is the correct way to do it? thanks again for the help!

@MikeGoldsmith
Copy link
Member

MikeGoldsmith commented May 10, 2023

You'll need to update the offsets.json after adding the new field. This can be done using make docker-offsets.

@oxeye-gal
Copy link
Contributor Author

I have added the new field to offsets-tracker, I hope this fixes it @MikeGoldsmith

@MikeGoldsmith
Copy link
Member

We're having hard time with this one 👎🏻 I'll give it a go too

…trumentation into http_server_context_propagation

# Conflicts:
#	CHANGELOG.md
#	pkg/instrumentors/bpf/net/http/server/bpf/probe.bpf.c
#	pkg/instrumentors/bpf/net/http/server/bpf_bpfel_arm64.go
#	pkg/instrumentors/bpf/net/http/server/bpf_bpfel_x86.go
#	pkg/instrumentors/bpf/net/http/server/probe.go
@MikeGoldsmith
Copy link
Member

@open-telemetry/go-instrumentation-approvers this is ready for another review 👍🏻

@MikeGoldsmith MikeGoldsmith mentioned this pull request May 23, 2023
2 tasks
Copy link
Contributor

@edeNFed edeNFed left a comment

Choose a reason for hiding this comment

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

lgtm

@MrAlias MrAlias merged commit f75c947 into open-telemetry:main May 23, 2023
@MrAlias MrAlias mentioned this pull request Jul 6, 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.

7 participants