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

Allow interceptors to yield and update request arguments #17651

Closed
wants to merge 1 commit into from

Conversation

blowmage
Copy link
Contributor

@blowmage blowmage commented Jan 7, 2019

This change allows Ruby interceptors to modify request values. (Response values are currently passed through the interceptor, and we have no issues modifying those values.) This was encountered while working on supporting OpenCensus in gRPC requests/responses. Specifically, streaming requests/responses are passed as Enumerable values. The approach we would like to take is to wrap the Enumerable with a Lazy Eumerable that will log when a request or response passes through the gRPC client. However, the current implementation does not allow the request Enumerable to be modified.

This change does not pass all the tests. There are some tests that specify that the request is opened before the values are passed through the interceptors. I don't know how to keep this behavior while still allowing interceptors to wrap the request Enumerable. Therefore I am opening this PR to start a discussion on how to move forward.

@blowmage
Copy link
Contributor Author

blowmage commented Jan 7, 2019

We would also like the interceptor to be able to set the OpenCensus gRPC metadata, as well as wrap the streaming requests/responses.

@blowmage
Copy link
Contributor Author

blowmage commented Feb 6, 2019

@apolcyn Have you had a chance to look at this?

@apolcyn
Copy link
Contributor

apolcyn commented Feb 12, 2019

sorry @blowmage this and #17650 slipped past me. Taking a look

Copy link
Contributor

@apolcyn apolcyn left a comment

Choose a reason for hiding this comment

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

Sorry I think I'm missing something here - the test that I see failing is

1) ClientStub #server_streamer via a call operation should send metadata to the server ok when start_call is run first
     Failure/Error: expect(c.metadata[k.to_s]).to eq(v)
       expected: "v1"
            got: nil

to be sure, is this the test in question?

@apolcyn
Copy link
Contributor

apolcyn commented Feb 15, 2019

@blowmage I should add, can we please add a test here? A test will help my understanding of this change for review. Regarding your comment, if we can find a backwards compatible way to make this change, then it should be ok; if it's breaking, we can evaluate. However, if this becomes a significant API addition or a breaking change, we'll need to at least at some point have a gRFC for too.

@blowmage
Copy link
Contributor Author

Yeah, I'm still trying to get the grpc ruby code to build and pass cleanly on OS X. I may have to bail on OS X and move this to a Linux VM.

Which Kokoro build includes the Ruby tests?

@blowmage
Copy link
Contributor Author

I see ruby test output in the "Basic Tests Multi-language Linux (internal CI)" build, but I don't see the ruby test output in any of the builds for MacOS or Windows.

@stale
Copy link

stale bot commented Sep 4, 2019

This issue/PR has been automatically marked as stale because it has not had any update (including commits, comments, labels, milestones, etc) for 180 days. It will be closed automatically if no further update occurs in 1 day. Thank you for your contributions!

@@ -334,17 +338,18 @@ def server_streamer(method, req, marshal, unmarshal,
if return_op
# return the operation view of the active_call; define #execute
# as a new method for this instance that invokes #server_streamer
c.merge_metadata_to_send(metadata)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the call that is causing the tests to fail. The metadata is sent before the interceptors have a chance to run and add additional metadata values.

@stale
Copy link

stale bot commented Mar 2, 2020

This issue/PR has been automatically marked as stale because it has not had any update (including commits, comments, labels, milestones, etc) for 180 days. It will be closed automatically if no further update occurs in 1 day. Thank you for your contributions!

@stale stale bot closed this Mar 9, 2020
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

4 participants