Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Expose request method of unary requests to clients and server handlers #502
Expose request method of unary requests to clients and server handlers #502
Changes from 2 commits
35e3b32
198c746
19d0e12
cc96335
bc5d7db
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NewClient.unaryFunc
(just before the block you've already added) and inClient.CallServerStream
. Server-side, we'd need to set the method explicitly inNewServerStreamHandler
. Then we could remove the block below that's special-casing the zero value.Method
inconnect-go
because it's so ambiguous - are we talking about a method on a protobuf RPC service, or are we talking about HTTP methods? I choseProcedure
for the former. Maybe we could name thisHTTPMethod()
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like you mention, could be useful for metrics or logs. It's just to provide some observability of under-the-hood mechanics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to
HTTPMethod()
and also updated the behavior and docs (and verified in tests) that it returns empty string until the method is actually known (after RPC is sent).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these interfaces need to embed
StreamingHandlerConn
andStreamingClientConn
...and do we need them at all? I think we could get away with anonymousfoo.(interface { getMethod() string})
-style casts instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I redid this. This is now using an anonymous interface as suggested for the handler side. For the client-side, to implement the new semantics (always blank until after RPC is sent, and set after), we need to know when the RPC is sent for all client conn impls. So now they all implement it, and there's no need for a type assertion.