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

Support google.golang.org/grpc v1.69.0 #1421

Open
MrAlias opened this issue Dec 13, 2024 · 7 comments · May be fixed by #1431
Open

Support google.golang.org/grpc v1.69.0 #1421

MrAlias opened this issue Dec 13, 2024 · 7 comments · May be fixed by #1431
Assignees
Milestone

Comments

@MrAlias
Copy link
Contributor

MrAlias commented Dec 13, 2024

The function google.golang.org/grpc/internal/transport.(*http2Server).WriteStatus was renamed to function google.golang.org/grpc/internal/transport.(*http2Server).writeStatus: https://github.com/grpc/grpc-go/blob/317271b232677b7869576a49855b01b9f4775d67/internal/transport/http2_server.go#L1045

Originally posted by @MrAlias in #1416 (comment)

@MrAlias MrAlias self-assigned this Dec 13, 2024
@MrAlias MrAlias added this to the v0.20.0-alpha milestone Dec 13, 2024
@MrAlias
Copy link
Contributor Author

MrAlias commented Dec 13, 2024

Looking at updating our definition of probe.Uprobe to handle this behavior split.

@damemi
Copy link
Contributor

damemi commented Dec 13, 2024

@MrAlias what are you thinking for the split? We have the MinVersion struct right now but I could see that getting messy, especially for situations where it's just a rename. Do you have an idea?

@MrAlias
Copy link
Contributor Author

MrAlias commented Dec 13, 2024

I'm still prototyping, but the current idea is to update the Uprobe with a version matching criteria and evaluate this during loadProbe.

@MrAlias
Copy link
Contributor Author

MrAlias commented Dec 14, 2024

It looks like handleStream's signature also changed: https://github.com/grpc/grpc-go/blob/317271b232677b7869576a49855b01b9f4775d67/server.go#L1735

Still working on it.

@MrAlias
Copy link
Contributor Author

MrAlias commented Dec 16, 2024

Update: The parent span context is no longer being found for the server spans. Digging into why.

@MrAlias
Copy link
Contributor Author

MrAlias commented Dec 16, 2024

Update: The parent span context is no longer being found for the server spans. Digging into why.

http2Server.operateHeaders also changed: grpc/grpc-go@2a18bfc#diff-4058722211b8d52e2d5b0c0b7542059ed447a04017b69520d767e94a9493409eL362-R362

This is where the parent span context is supposed to come from. Updating.

@MrAlias
Copy link
Contributor Author

MrAlias commented Dec 16, 2024

Update: The parent span context is no longer being found for the server spans. Digging into why.

http2Server.operateHeaders also changed: grpc/grpc-go@2a18bfc#diff-4058722211b8d52e2d5b0c0b7542059ed447a04017b69520d767e94a9493409eL362-R362

This is where the parent span context is supposed to come from. Updating.

This looks to have been my mistake in reading the embedded Stream correctly from ServerStream.

MrAlias added a commit to MrAlias/opentelemetry-go-instrumentation that referenced this issue Dec 16, 2024
@MrAlias MrAlias linked a pull request Dec 16, 2024 that will close this issue
MrAlias added a commit to MrAlias/opentelemetry-go-instrumentation that referenced this issue Dec 16, 2024
MrAlias added a commit to MrAlias/opentelemetry-go-instrumentation that referenced this issue Dec 16, 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
Development

Successfully merging a pull request may close this issue.

2 participants