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

(configgrpc): configure memorylimiterextension when setting up grpc servers #9673

Conversation

moh-osman3
Copy link
Contributor

@moh-osman3 moh-osman3 commented Mar 3, 2024

Description:

This PR adds a new configgrpc option to configure a memory limiter extension to use. This allows users to leverage the new memorylimiterextension within receivers that set up a gRPC server.

Link to tracking Issue:

This is part of #9591 and relates to #8632

Testing:

Added new unit tests that cover the added code.

Copy link

codecov bot commented Mar 3, 2024

Codecov Report

Attention: Patch coverage is 89.65517% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 90.93%. Comparing base (e696206) to head (bde885b).
Report is 195 commits behind head on main.

Files Patch % Lines
config/configgrpc/configgrpc.go 89.65% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9673      +/-   ##
==========================================
- Coverage   90.93%   90.93%   -0.01%     
==========================================
  Files         347      348       +1     
  Lines       18346    18408      +62     
==========================================
+ Hits        16683    16739      +56     
- Misses       1340     1346       +6     
  Partials      323      323              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@moh-osman3 moh-osman3 force-pushed the mohosman/configgrpc-support-memorylimiter-extension branch from 8752b44 to 95c8707 Compare March 4, 2024 21:01
@moh-osman3 moh-osman3 force-pushed the mohosman/configgrpc-support-memorylimiter-extension branch from 34b51ea to bde885b Compare March 5, 2024 06:14
@moh-osman3 moh-osman3 marked this pull request as ready for review March 5, 2024 07:49
@moh-osman3 moh-osman3 requested review from a team and Aneurysm9 March 5, 2024 07:49
return memoryLimiterUnaryServerInterceptor(ctx, req, info, handler, memoryLimiter)
})
sInterceptors = append(sInterceptors, func(srv any, ss grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) error {
return memoryLimiterStreamServerInterceptor(srv, ss, info, handler, memoryLimiter)
Copy link
Contributor Author

@moh-osman3 moh-osman3 Mar 5, 2024

Choose a reason for hiding this comment

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

This path would require setting up streaming server/client to increase unit test coverage. This doesn't seem necessary because I already have coverage for memoryLimiterStreamServerInterceptor.

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added Stale and removed Stale labels Mar 20, 2024
Copy link
Contributor

github-actions bot commented Apr 4, 2024

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added Stale and removed Stale labels Apr 4, 2024
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Apr 20, 2024
Copy link
Contributor

github-actions bot commented May 8, 2024

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant