transport: writeStatus: Skip Status.Proto() without details

[evanj]

Add Status.HasDetails() to check for details. Change http2server.writeStatus to check to avoid unnecessary clones in the common case when there are no details. This seems to use about 0.3% of CPU for a high QPS gRPC service, according to the Go profiler.

While looking at profiles for a high QPS gRPC service at Datadog, I noticed that about 0.3% of CPU time is being spent calling Status.Proto(). I believe this is unnecessary: this clones the protobuf status message, only to check if there are details. If there are not (the common case, e.g. for status code OK), it throws the protobuf message away.

I tried quickly but was not able to reliably measure the impact of this change from any end-to-end benchmarks, but it does seem to make this function go away according to the profiler. This should be a small win for every gRPC server.

RELEASE NOTES:

• server: improve performance of server RPC handling by avoid a status proto copy

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.10%. Comparing base (080f956) to head (792ed4c).
Report is 9 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8282      +/-   ##
==========================================
- Coverage   82.13%   82.10%   -0.03%     
==========================================
  Files         417      419       +2     
  Lines       41385    41946     +561     
==========================================
+ Hits        33991    34440     +449     
- Misses       5961     6030      +69     
- Partials     1433     1476      +43     

Files with missing lines	Coverage Δ
internal/status/status.go	91.00% <100.00%> (+0.47%)	⬆️
internal/transport/http2_server.go	90.36% <100.00%> (-0.73%)	⬇️

... and 34 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dfawley]

This changes our external API since grpc/status/Status is a type alias of grpc/internal/Status/Status. It may be preferable to implement a grpc/internal/status/RawStatusProto(s *Status) *sbp.Proto that just returns the internals, and then operate on them directly instead. The reason we split status in two places is because we want to be able to do things like this that would be unsafe for users to directly access, but improves efficiency of our internal code.

[evanj]

I have updated the PR to add the suggested RawStatusProto function instead with a test (although the test is pretty silly and could be removed).

I'm happy to make any other suggested improvements. Thanks for the suggestion and the code review!

One unimportant ignorable thought: Another option would be to make this internal function more restricted: HasDetails(...) bool instead. This would avoid revealing the entire internals of the Status type. This seems fairly unimportant. I went with RawStatusProto because I have WAY less context on this project.

[dfawley]

Thanks!

[dfawley]

@arjan-bal can you take a second pass please?

[evanj]

I have added a copyright to the new test file by copy/pasting and editing the header from status.go in an attempt to fix vet.sh.

[arjan-bal]

Minor comments, otherwise LGTM.

[evanj]

I believe I addressed all the code review comments. I resolved all the line comments above, and unassigned myself. Happy to address any other feedback!