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.

Sign up for GitHub

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

Jump to bottom

Update net/http instrumentation to comply with semantic conventions #790

Merged
merged 30 commits into from
May 14, 2024
Merged

Update net/http instrumentation to comply with semantic conventions #790

merged 30 commits into from
May 14, 2024

Conversation

damemi
Copy link
Contributor

@damemi damemi commented Apr 18, 2024 •
edited
Loading

Fixes #169

This does the following:

  • Sets span.Status for 4xx and 5xx codes in Client, and just 5xx codes in Server
  • Sets protocol.name when the protocol is not HTTP for both Client and Server
  • Generates a url.full attribute from the protocol+host+path for Client
  • Adds Probe tests to Client and Server instrumentation for changes
  • Adds a new opentelemetry/controller_test.go to check that the output gets exported as expected
  • Updates fixtures for HTTP and Gin to show new url.full field

This PR adds changes to the net/http eBPF instrumentation to copy the full set of fields from the *net/url.URL object in the Request to reconstruct a url.full attribute. It copies each field individually because the object is a pointer to an address in user space which would not be accessible by the auto-instrumentation agent. The agent reconstructs the Go URL object so that it can call url.String(), which uses logic and control flags in the original object to build the string representation (rather than building the string manually in the eBPF program).

@damemi damemi requested a review from a team April 18, 2024 16:48
@damemi damemi mentioned this pull request Apr 22, 2024
Closed
MrAlias
MrAlias reviewed Apr 24, 2024
View reviewed changes
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Overall looks good. Just the full URL and semconv versioning look like the only items that need to be addressed.

CHANGELOG.md Outdated Show resolved Hide resolved
internal/pkg/instrumentation/bpf/net/http/client/probe.go Outdated Show resolved Hide resolved
internal/pkg/instrumentation/bpf/net/http/client/probe_test.go Outdated Show resolved Hide resolved
internal/pkg/instrumentation/bpf/net/http/server/probe.go Outdated Show resolved Hide resolved
internal/pkg/instrumentation/bpf/net/http/server/probe_test.go Outdated Show resolved Hide resolved
internal/pkg/opentelemetry/controller_test.go Outdated Show resolved Hide resolved
@RonFed RonFed mentioned this pull request Apr 30, 2024
Merged
@damemi damemi mentioned this pull request May 3, 2024
Open
@damemi damemi force-pushed the issue-169 branch 3 times, most recently from 3352728 to 3401852 Compare May 6, 2024 17:43
@damemi
Copy link
Contributor Author

damemi commented May 6, 2024

I think test failure is unrelated and fixed by #820

@damemi damemi force-pushed the issue-169 branch 4 times, most recently from 7a180bd to 68e6874 Compare May 8, 2024 13:50
@damemi damemi mentioned this pull request May 8, 2024
Open
MrAlias
MrAlias approved these changes May 8, 2024
View reviewed changes
CHANGELOG.md Show resolved Hide resolved
@MrAlias
Copy link
Contributor

MrAlias commented May 8, 2024

@RonFed @grcevski please take another look at this and leave a review.

grcevski
grcevski approved these changes May 8, 2024
View reviewed changes
Copy link
Contributor

@grcevski grcevski left a comment

Choose a reason for hiding this comment

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

LGTM!

RonFed
RonFed approved these changes May 8, 2024
View reviewed changes
Copy link
Contributor

@RonFed RonFed left a comment

Choose a reason for hiding this comment

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

Added a comment about the Makefile change, other than that looks great.

Makefile Show resolved Hide resolved
damemi added 22 commits May 14, 2024 16:42
@damemi
make fixture-gin
fe5f653
@damemi
Update changelog
2968339
@damemi
Add protocol.name to server when not HTTP
92ab45c
@damemi
Fix lint
57a5a99
@damemi
Add go.work* to gitignore
e5624b1
@damemi
Add url.URL fields to net/http offsetgen
93df8f7
@damemi
Add -buildvcs=false to offsets/tools
6a085a7
@damemi
Add net/url.URL fields to offsetgen
588493c
@damemi
Update probe.go and add more tests for URL
7411d71
@damemi
make precommit
921d4d3
@damemi
make fixture-nethttp-custom
e62a7f9
@damemi
make fixture-gin
21a8868
@damemi
make fixture-databasesql
b361267
@damemi
Remove inject_header (must have been re-added during rebase)
fb3819c
@damemi
make docker-offsets
f28a781
@damemi
feedback
758bf84
@damemi
rename deprecated semconv key
e84d9e0
@damemi
Download cached offset_results.json
251ec4d
@damemi
re-run make docker-offsets with cached offsets
b6e6583
@damemi
Use multiples of 8 for field sizes
71ac5f3
@damemi
make docker-offsets
bc468f7
@damemi
Add verify.bats entry for url.full
fab659d
@MrAlias MrAlias added this to the v0.12.0-alpha milestone May 14, 2024
@damemi
Copy link
Contributor Author

damemi commented May 14, 2024

ready to merge @MrAlias

@MrAlias MrAlias merged commit e85ddd9 into open-telemetry:main May 14, 2024
17 checks passed
@MrAlias MrAlias modified the milestones: v0.12.0-alpha, v0.13.0-alpha Jun 4, 2024
@MrAlias MrAlias mentioned this pull request Jun 4, 2024
Merged
@damemi damemi mentioned this pull request Jun 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrAlias MrAlias MrAlias approved these changes

@RonFed RonFed RonFed approved these changes

@grcevski grcevski grcevski approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Go Auto Instrumentation: Beta
Status: Done
Milestone
v0.13.0-alpha
Development

Successfully merging this pull request may close these issues.

Audit and update net/http instrumentation to comply with HTTP semantic conventions
4 participants
@damemi @MrAlias @grcevski @RonFed