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

Closed
wants to merge 1 commit into 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/grpc@v1.69.0-dev depends on google.golang.org/genproto/googleapis/rpc@v0.0.0-20240903143218-8af14fe29dc1:

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/rpc@v0.0.0-20240903143218-8af14fe29dc1/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

@MrAlias
Copy link
Contributor

MrAlias commented Nov 5, 2024

Tracking issue in #1226. Closing.

@MrAlias MrAlias closed this Nov 5, 2024
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