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 #1431

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Dec 16, 2024

Resolve #1421
Resolve #1432

In google.golang.org/grpc v1.69.0 the http2Server.writeStatus and Server.handleStream method signatures changed:

In both cases, they are now passed *transport.ServerStream instead of *transport.Stream. The v1.69.0 version of transport.Stream hasn't changed, just how it is referenced. In v1.69.0 the *Stream is embedded in ServerStream.

This PR handles this divergence and support both code paths.

Split uprobes

To reduce complexity of the uprobes by adding another volatile flag variable, this splits functionality of the uprobes. A new PackageConstraints field is added to the probe.Uprobe to provide scope of when the probe should be loaded and the behavior when it is not loaded.

This "behavior when not loaded" concept overlaps with then already existing Optional field of the Probe. This is replace with a more customizable FailureMode type that describes the behavior.

The common functionality of the uprobes are abstracted to inlined functions.

Event type updates

The event type now reports if the status was set.

Go context parsing

The Go context.Context is held within the Stream.ctx field. This is not directly accessible via passed arguments of the instrumented methods; it is only accessable via ServerStream.Stream.ctx, there is an added level of indirection. This added level of indirection means for the updated code paths our header file functionality is not used.

google.golang.org/grpc v1.68.1

This was manually tested against google.golang.org/grpc v1.68.1 by downgrading the version of grpc used in our e2e tests and tested.

@MrAlias MrAlias force-pushed the grpc-1.69.0 branch 3 times, most recently from 0149c1c to 0018d16 Compare December 16, 2024 21:46
@MrAlias MrAlias changed the title [PoC] Support grpc 1.69.0 Support google.golang.org/grpc v1.69.0 Dec 16, 2024
@MrAlias MrAlias marked this pull request as ready for review December 16, 2024 22:28
@MrAlias MrAlias requested a review from a team as a code owner December 16, 2024 22:28
@MrAlias MrAlias force-pushed the grpc-1.69.0 branch 2 times, most recently from 1a3e07f to 6d97cc2 Compare December 17, 2024 00:52
Replace the Optional field with a more expressive FailureMode and the
addition of PackageConstrainsts to restrict when the uprobe should be
loaded.
The handleStream function supports grpc < 1.40.0.

Reuse the serverStream const for all constraints.
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.

@MrAlias Do you think we should do the test we talked about yesterday to make sure the context reading is OK? (creating a child span for the gRPC server span)

This looks great to me

@MrAlias
Copy link
Contributor Author

MrAlias commented Dec 18, 2024

@MrAlias Do you think we should do the test we talked about yesterday to make sure the context reading is OK? (creating a child span for the gRPC server span)

This looks great to me

I was tracking that work in #1442.

I was unsure if we wanted to add it here as this PR is already quite large. I can add it as a follow up, and even add it in a branch based on this branch to validate this will work. Or, I can just add it to this PR. What is your preference @RonFed?

@MrAlias
Copy link
Contributor Author

MrAlias commented Dec 18, 2024

@RonFed please take a look at #1468. Let me know if you think that should be combined with this PR.

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 this pull request may close these issues.

Remove unused peer_ptr in grpc server probe Support google.golang.org/grpc v1.69.0
2 participants