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

Add net/http client instrumentor #91

Merged
merged 18 commits into from
Jun 14, 2023

Conversation

oxeye-gal
Copy link
Contributor

Hi,

This PR attempts to add instrumentation for net/http client.

We do it by writing a fake tophash value (as calculating the correct one will be harder to achieve) to the first headers map bucket and writing the "traceparent" header key and value into memory.

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 which in turn sends an http request to an additional instrumented Go http server.

The produced trace looks like the following:

213374473-d266bf1f-49f5-4f8c-8e0d-53ed51d80201

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

@oxeye-gal oxeye-gal requested a review from a team as a code owner April 24, 2023 06:51
@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.

@pellared
Copy link
Member

Hey @oxeye-gal, are you able to sign the CLA?

@pellared pellared changed the title Adding net/http client instrumentor Add net/http client instrumentor Apr 24, 2023
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.

@oxeye-gal looks like this updates the test fixtures (which is expected since this is a new instrumentation) based on the CI failure

If you have kind installed, you should be able to run make fixture-nethttp to re-generate the test output and commit the result.

@oxeye-gal
Copy link
Contributor Author

Done. @damemi

@damemi
Copy link
Contributor

damemi commented Apr 27, 2023

Sorry, need to run make update-licenses and make fixture-gorillamux

@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.

Merge upstream main, add changelog and make small fixes
@oxeye-gal oxeye-gal force-pushed the http_client_instrumentation branch from 7878f9b to d59e91b Compare May 10, 2023 14:26
@MikeGoldsmith
Copy link
Member

Hmm - not sure what happened there. I'll take a look when I can.

@MikeGoldsmith
Copy link
Member

@oxeye-gal now #92 has been merged, I’ll look to update this PR too 👍🏻

@MikeGoldsmith
Copy link
Member

Picking this up now - sorry it's taken longer than expected to get back to.

…mentation into http_client_instrumentation

# Conflicts:
#	CHANGELOG.md
#	test/e2e/gin/traces.json
#	test/e2e/gorillamux/traces.json
#	test/e2e/nethttp/traces.json
@MikeGoldsmith MikeGoldsmith self-assigned this Jun 6, 2023
…trumentation into http_client_instrumentation

# Conflicts:
#	CHANGELOG.md
#	Makefile
@MikeGoldsmith
Copy link
Member

PR to update @oxeye-gal's PR is ready for merge, once it's done, we can review & merge this PR 🎉

Update upstream PR to add HTTP client propagation support
@MikeGoldsmith
Copy link
Member

@oxeye-gal please could you "update branch" one more time, and we can merge this PR :)

@MikeGoldsmith MikeGoldsmith dismissed damemi’s stale review June 12, 2023 11:32

Requested changes have been made, I will be updating to consolidate probes once merged

@MikeGoldsmith
Copy link
Member

MikeGoldsmith commented Jun 12, 2023

@open-telemetry/go-instrumentation-approvers Please can you review this PR - it needs a final "update branch" but once done I'd like to merge this. I've worked on making sure it works and plan on expanding the same work to the other HTTP probes once it's done (see #101).

@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

@MikeGoldsmith MikeGoldsmith merged commit a1cff32 into open-telemetry:main Jun 14, 2023
11 checks passed
@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.

None yet

6 participants