gRPC integration and aio interceptors

[fdellekart]

This PR builds on the changes pushed to #2055 . The commits from there were squashed and reworded to adhere to standards described in the contributing guidelines.

Overall, the following functionality was added:

• Client and server interceptors to instrument gRPC calls.
• A gRPC integration to monkeypatch the gRPC functions used for channels and servers.

What's still missing:

• unary_stream interceptor for the client, I didn't yet get to looking into that into detail.

[fdellekart]

I made the changes you suggested @alisoam , thx for raising 👍 😄

Started writing the unary-stream client interceptor for async also, however, I would need some clarification on this topic. I am not extremely familiar with those functionalities and would rather first coordinate.

Is the behavior implemented in the sync version for unary-stream the desired one? It creates one span when the request is incoming which ends as soon as the UnaryStreamCall object obtained from the continuation is returned. However, this is only half the story, because AFAIU later the client will iterate over that object to obtain the individual messages from the server.

So I think the solution could be to return a custom UnaryStreamCall object which wraps also the read method of the underlying object into a span. Due to my atm quite limited understanding of the internals of this, this is mostly guesswork, however.

If there's somebody here who can give me a bit more of an insight I can add it to this PR, else I'd suggest opening a follow up issue for the other types of calls (stream-unary and stream-stream were not touched yet at all) 👍 😉

[antonpirker]

Hey! I am looking at this now. One first thing, could you fix the type errors in the failed check CI / Lint Sources? (You can run the linting locally with ./scripts/runtox.sh linters. At least I hope this works in your local environment)

[antonpirker]

@fdellekart About your question about the "sync version for unary-stream" behavior. I am really not deep into grpc. But I guess for the first version this behavior should be OK. We can always release an update to the integration to improve things. For now I would say we try to bring this out the door.

[antonpirker]

I am trying to test this and I have a example script resembling this: https://stackoverflow.com/a/63020695/300572

And I get this error:

grpc.aio._call.AioRpcError: <AioRpcError of RPC that terminated with:
        status = StatusCode.UNKNOWN
        details = "Unexpected <class 'TypeError'>: object HelloReply can't be used in 'await' expression"
        debug_error_string = "UNKNOWN:Error received from peer  {created_time:"2023-09-27T16:03:03.322818+02:00", grpc_status:2, grpc_message:"Unexpected <class \'TypeError\'>: object HelloReply can\'t be used in \'await\' expression"}"

Can you tell me what I am doing wrong @fdellekart ?

[antonpirker]

One question @fdellekart or @alisoam

Previously users had to set the interceptors by hand (see https://docs.sentry.io/platforms/python/configuration/integrations/grpc/) and with this update this is done automatically, if the GRPCIntegration is added.

If people add now the new GRPCIntegration and forget to remove the "adding interceptors by hand", will this be a problem? Will then be two interceptors do the same thing? Can we make this somehow backwards compatible?

Of if there is already the Sentry interceptor present, do not add it again (when calling grpc.intercept_channel() and in the new _wrap_*() functions)?

[fdellekart]

Thank you for the clarifications, I already fixed the linters and added the async interceptor for unary-stream, will add some more tests and then push everything in the upcoming days.

Regarding the error described above, I started the greeter service and client from the official gRPC examples with the integration and I unfortunately can't reproduce your error. What script are you using as the server? As you are getting a response I assume the issue is there somewhere.

Good point regarding the backwards compatibility, I will add some logic to only add the interceptor when not yet present 👍

[fdellekart]

@antonpirker I now fixed the described issues and also added tests to avoid that the integration breaks functionality of currently unsupported RPC types.

The other RPC types apart from unary-unary and unary-stream are now only implemented so that the integration works with them, but there is not yet any tracing supported, whilst the second one is only supported on the client side. I think for now that's enough and from my side, this is good to go to not let this PR grow bigger than it must be.

As soon as I get to it I will open a follow-up PR to implement tracing and error handling for the other RPC types if nobody else does so before me 👍 😃

AFAIU I cannot trigger the test and linters pipelines myself, so let me know if there's anything I can still help with after those have run 👍

[fdellekart]

Also on the last commit I pushed I found a bug in the sync version of the gRPC unary stream client interceptor. It's getting stuck when trying to add the status code to the span. No clue if this worked with older gRPC versions because it was not tested yet.

On reply to that: There's an issue open in grpc on that topic already, for now we can't have the status code on the streaming response because of it.

[antonpirker]

Thanks for the update!
I fixed some smaller linting issues.
I will probably look at this tomorrow, because I am in the middle of something right now.

[antonpirker]

Sorry for not yet looking at this. I am finishing up something else and then I will look at this. Hopefully this week.

[antonpirker]

Hey @fdellekart

Sorry, was caught up with other stuff, but I try to push this over the finish line until tomorrow.

[antonpirker]

I made a change, to have nicer transaction names in the async server intercepter. @fdellekart can you have a look at the commit: 7be1306 and tell me if having this _find_name not a static method but a normal method can cause any problems?

[antonpirker]

Also updated the docs: getsentry/sentry-docs#8434

[fdellekart]

I made a change, to have nicer transaction names in the async server intercepter. @fdellekart can you have a look at the commit: 7be1306 and tell me if having this _find_name not a static method but a normal method can cause any problems?

Shouldn't be a problem 👍

[antonpirker]

Nice! So now everything is ready. We on our end just need to fix the problem with the aws tests (they use github secrects and PRs from outside our organization can not access them)

As soon as we have this figured out we can merge and it will be available in the next release (which will be this or next week)!

[antonpirker]

looks very good now! 💯