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

feat(integrations): Add integerations for grpc.aio #2055

Closed
wants to merge 3 commits into from
Closed

feat(integrations): Add integerations for grpc.aio #2055

wants to merge 3 commits into from

Conversation

alisoam
Copy link

@alisoam alisoam commented Apr 28, 2023

Add server interceptor for asyncio grpc servers.

ali.sorouramini added 3 commits April 29, 2023 08:31
Capture unhandled exceptions in the async grpc server interceptor.
@alisoam alisoam marked this pull request as draft May 5, 2023 17:59
@github-actions
Copy link

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@alisoam
Copy link
Author

alisoam commented Aug 31, 2023

Hi,
I'm glad that this MR is open again. :))))
I have the client interceptor on my laptop.
I will add the unary_unary client interceptor in following days.
I'm not sure these interceptors would work with stream calls correctly.

@fdellekart
Copy link
Contributor

@alisoam FYI: As this PR was already 5 months old, I didn't expect somebody to continue working on it so soon. Therefore, I pulled your branch, added the unary_unary aio client interceptors, some tests, and also an integration which monkeypatches the relevant grpc functions.

If you can use anything from there feel free to do so and cherry-pick the commits, of course I don't want to overrule your efforts here 😉

I pushed the changes here. Will then submit the integration in an extra PR later 👍 😄

@antonpirker
Copy link
Member

Hey @alisoam and @fdellekart !

Yea, sorry for it taking so long. But I will now have time to look at it.
Should I just review this PR or merge something from @fdellekart into it? (If yes, can you tell me what commits need to end up in this PR?)

Thanks to both of you!

@fdellekart
Copy link
Contributor

I merged the current branch into mine and squashed/reworded the commits a bit to adhere to the formats described in the contribution guidelines.

Therefore, we could actually just continue this there if that's ok for everybody. I opened #2369 for this purpose 👍

@alisoam
Copy link
Author

alisoam commented Sep 14, 2023

Hi @fdellekart,
I believe your PR is easier to merge because of the merge conflicts in this one. The implementation of Client Interceptors appears quite similar. I've added some comments to your PR. I suggest we close this PR and continue the discussion there.

@fdellekart
Copy link
Contributor

Perfect, thx 👍 I am currently on vacation until mid next week but will have a look on your comments afterwards 🙂

@antonpirker
Copy link
Member

Ok, very cool @alisoam and @fdellekart !
I will close this PR in favor of this one: #2369

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants