Skip to content

Conversation

yordis
Copy link
Contributor

@yordis yordis commented Oct 1, 2025

Signed-off-by: Yordis Prieto [email protected]

@yordis yordis marked this pull request as ready for review October 1, 2025 18:51
@yordis yordis force-pushed the yordis/add-grpc branch 10 times, most recently from 2f685df to 396975f Compare October 3, 2025 18:12
@yordis yordis requested a review from sleipnir October 3, 2025 19:08
@yordis yordis force-pushed the yordis/add-grpc branch 8 times, most recently from 1cb549c to 14faa20 Compare October 3, 2025 19:43
Copy link

@sleipnir sleipnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sleipnir
Copy link

sleipnir commented Oct 3, 2025

@yordis I'm not a maintainer of this project, but PR looks good to me

@yordis yordis force-pushed the yordis/add-grpc branch 4 times, most recently from d553114 to 173ba90 Compare October 3, 2025 23:13
@yordis yordis force-pushed the yordis/add-grpc branch 6 times, most recently from 9a397be to 4516097 Compare October 4, 2025 01:12
@yordis yordis requested a review from sleipnir October 4, 2025 01:12
@yordis
Copy link
Contributor Author

yordis commented Oct 4, 2025

@sleipnir do you mind giving it another code review; by now

Copy link

@sleipnir sleipnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just minor comments, we're getting there, congratulations.

What do you think about adding a small benchmark, comparing the interceptor for client and server? If it's not too much work, of course.

@yordis
Copy link
Contributor Author

yordis commented Oct 4, 2025

@sleipnir thank you so much for the review; I am gonna wait for @tsloughter input to tackle the comments and other things, some of them are there to actually help me to make sense of things, I honestly keep getting confused with Linking vs Child; and a nice pattern to follow

@tsloughter
Copy link
Member

Thanks for reviewing @sleipnir

@tsloughter
Copy link
Member

It doesn't have to be this way, but it might be a little cleaner if everything was done in the interceptors, instead of creating the spans in telemetry events and doing context propagation in the interceptor.

@yordis
Copy link
Contributor Author

yordis commented Oct 5, 2025

@tsloughter the downside of doing that is that, you need to register the interceptor for every single client;
I can write an interceptor, but I would still have a way to enable things globally in the system so developers do not need to worry about it

@sleipnir
Copy link

sleipnir commented Oct 6, 2025

Thanks for reviewing @sleipnir

You're welcome <3

@yordis yordis force-pushed the yordis/add-grpc branch 10 times, most recently from a5e534a to df85958 Compare October 9, 2025 02:53
@yordis yordis requested a review from tsloughter October 9, 2025 02:53
@yordis yordis force-pushed the yordis/add-grpc branch 2 times, most recently from 2ca2202 to 478b5db Compare October 11, 2025 06:35
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.

3 participants