-
Notifications
You must be signed in to change notification settings - Fork 105
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
Conversation
@@ -1679,7 +1669,8 @@ | |||
"1.7.2", | |||
"1.7.3", | |||
"1.7.4", | |||
"1.7.5" | |||
"1.7.5", | |||
"1.69.0-dev" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks suspicious
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, odd... this version was also wholly removed from another section below https://github.com/open-telemetry/opentelemetry-go-instrumentation/pull/1206/files#diff-15a322f23067198dc3f6baea35decb97f02a703fe637d6191e9871c5bf43d564L3793-L3796
There was a problem hiding this comment.
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:
How was this generated?
There was a problem hiding this comment.
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(?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
Tracking issue in #1226. Closing. |
Fixes #1204
Not sure if this is worth backporting but if so I can take that