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

server: populate WireLength on stats.InPayload for unary RPCs #2932

Merged
merged 1 commit into from Jul 24, 2019

Conversation

ajwerner
Copy link
Contributor

Fixes #2692 which was incompletely fixed by #2711.

Also adds a basic unit test which provides scaffolding for further testing of
the stats logic.

@thelinuxfoundation
Copy link

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

Regards,
CLA GitHub bot

@ajwerner
Copy link
Contributor Author

@menghanl any chance I can bother you to review this PR to finish off fixing #2692?

Copy link
Contributor

@menghanl menghanl left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. The change looks good.

For the test, can you see if it can be done in stats_test.go instead?
Maybe a WireLength check here for in payload and here for out payload

Fixes grpc#2692 which was incompletely fixed by grpc#2711.

Also adds updates stats/stat_test.go to sanity check WireLength.
@ajwerner ajwerner force-pushed the ajwerner/fix-InPayload_WireLength branch from 1a729ca to 3e986df Compare July 24, 2019 22:03
@ajwerner
Copy link
Contributor Author

For the test, can you see if it can be done in stats_test.go instead?
Maybe a WireLength check here for in payload and here for out payload

Done. Much simpler.

Any hope we could backport this and #2711 to 1.21?

@menghanl menghanl merged commit b5748ca into grpc:master Jul 24, 2019
@menghanl
Copy link
Contributor

We can do a backport for this change.

Isn't #2711 already in 1.21?

@menghanl menghanl added this to the 1.23 Release milestone Jul 24, 2019
@ajwerner
Copy link
Contributor Author

We can do a backport for this change.
Awesome!

Isn't #2711 already in 1.21?

Yes, sorry, I've been getting my versions mixed up. I was looking at 1.18 but if we get this into 1.21 I'm happy. I'll make the PR.

ajwerner added a commit to ajwerner/grpc-go that referenced this pull request Jul 25, 2019
)

Fixes grpc#2692 which was incompletely fixed by grpc#2711.

Also adds updates stats/stat_test.go to sanity check WireLength.
menghanl pushed a commit that referenced this pull request Jul 25, 2019
Fixes #2692 which was incompletely fixed by #2711.

Also adds updates stats/stat_test.go to sanity check WireLength.
menghanl pushed a commit that referenced this pull request Jul 25, 2019
Fixes #2692 which was incompletely fixed by #2711.

Also adds updates stats/stat_test.go to sanity check WireLength.
@lock lock bot locked as resolved and limited conversation to collaborators Jan 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WireLength is not set on the stats.InPayload used as input to stats.Handler.HandleRPC()
3 participants