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: net/http incorrect offsets #1206

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

damemi
Copy link
Contributor

@damemi damemi commented Oct 21, 2024

Fixes #1204

Not sure if this is worth backporting but if so I can take that

@@ -1679,7 +1669,8 @@
"1.7.2",
"1.7.3",
"1.7.4",
"1.7.5"
"1.7.5",
"1.69.0-dev"
Copy link
Contributor

Choose a reason for hiding this comment

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

That looks suspicious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, yeah, this seems wrong. google.golang.org/[email protected] depends on google.golang.org/genproto/googleapis/[email protected]:

https://github.com/grpc/grpc-go/blob/4544b8a4cfe9bb4882ec3591631b83dac7434805/go.mod#L16

That version of that package definitely has a Status.Code field:

https://pkg.go.dev/google.golang.org/genproto/googleapis/[email protected]/status#Status

How was this generated?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I think our problem might be coming from the versioning issues of having one module holding another's code. The google.golang.org/genproto/googleapis/rpc module is separate from google.golang.org/grpc. It just doesn't have any versioned releases, which I'm guessing, is why it is not listed as its own module entry(?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I generated this by deleting my local .tools/ and offset_results.json then running make docker-offsets. Trying to see if I can get something more reasonable by updating the Go version in the image and tools

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Couldn't get any difference, but I checked back and it was actually the offset for http2client.nextID that was removed in this PR for that version: https://github.com/open-telemetry/opentelemetry-go-instrumentation/pull/1206/files#diff-15a322f23067198dc3f6baea35decb97f02a703fe637d6191e9871c5bf43d564L3791-L3796

And it was added to the null offset for that field on this line: https://github.com/open-telemetry/opentelemetry-go-instrumentation/pull/1206/files#diff-15a322f23067198dc3f6baea35decb97f02a703fe637d6191e9871c5bf43d564R3598-R3599

Looking more at this PR, it looks like it removed all the fields in 1.69.0-dev from having an offset to being null, so something's up there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants