yarpc / yarpc-go

A message passing platform for Go
MIT License
405 stars 103 forks source link

Add `AddHeader` for transport #2126

Closed jstroem closed 2 years ago

jstroem commented 2 years ago

The only outside facing change this PR comes with is that if an http header has Rpc-Header- as a prefix, like Rpc-Header-Foo we now don't add another Rpc-Header- as a prefix. (See tests in transport/http/header_test.go).

Benchmark results for AddHeaders pre vs post change:

name                                                         old time/op    new time/op    delta
pkg:go.uber.org/yarpc/transport/grpc goos:darwin goarch:amd64
_ResponseWriter_AddHeaders/lowercase                           1.14µs ± 2%    1.12µs ±10%      ~     (p=0.251 n=9+10)
_ResponseWriter_AddHeaders/lowercase_no-value                   165ns ± 2%     103ns ±11%   -37.37%  (p=0.000 n=9+9)
_ResponseWriter_AddHeaders/titlecase                           1.21µs ± 3%    1.18µs ±11%      ~     (p=0.280 n=10+10)
_ResponseWriter_AddHeaders/titlecase_no-value                   192ns ± 4%     149ns ±15%   -22.46%  (p=0.000 n=10+10)
pkg:go.uber.org/yarpc/transport/http goos:darwin goarch:amd64
_ResponseWriter_AddHeaders/lowercase                            345ns ±14%     394ns ±10%   +14.29%  (p=0.000 n=10+10)
_ResponseWriter_AddHeaders/titlecase                            357ns ± 9%     326ns ±11%    -8.69%  (p=0.021 n=9+10)
_ResponseWriter_AddHeaders/titlecase_with_rpc-header_prefix     413ns ± 4%     312ns ±15%   -24.46%  (p=0.000 n=10+10)
pkg:go.uber.org/yarpc/transport/tchannel goos:darwin goarch:amd64
_ResponseWriter_AddHeaders/lowercase                            124ns ±10%     131ns ± 3%    +5.37%  (p=0.021 n=10+9)
_ResponseWriter_AddHeaders/titlecase                            209ns ± 9%     187ns ± 9%   -10.54%  (p=0.000 n=10+10)

name                                                         old alloc/op   new alloc/op   delta
pkg:go.uber.org/yarpc/transport/grpc goos:darwin goarch:amd64
_ResponseWriter_AddHeaders/lowercase                             264B ± 0%      232B ± 0%   -12.12%  (p=0.000 n=10+10)
_ResponseWriter_AddHeaders/lowercase_no-value                   32.0B ± 0%      0.0B       -100.00%  (p=0.000 n=10+10)
_ResponseWriter_AddHeaders/titlecase                             279B ± 0%      247B ± 0%   -11.47%  (p=0.000 n=10+10)
_ResponseWriter_AddHeaders/titlecase_no-value                   35.0B ± 0%      3.0B ± 0%   -91.43%  (p=0.000 n=10+10)
pkg:go.uber.org/yarpc/transport/http goos:darwin goarch:amd64
_ResponseWriter_AddHeaders/lowercase                             117B ±10%      122B ± 7%      ~     (p=0.090 n=10+10)
_ResponseWriter_AddHeaders/titlecase                             119B ± 6%      107B ± 7%   -10.17%  (p=0.000 n=9+10)
_ResponseWriter_AddHeaders/titlecase_with_rpc-header_prefix      172B ± 8%      102B ± 8%   -40.82%  (p=0.000 n=10+10)
pkg:go.uber.org/yarpc/transport/tchannel goos:darwin goarch:amd64
_ResponseWriter_AddHeaders/lowercase                            0.00B          0.00B           ~     (all equal)
_ResponseWriter_AddHeaders/titlecase                            9.00B ± 0%     3.00B ± 0%   -66.67%  (p=0.000 n=10+10)

name                                                         old allocs/op  new allocs/op  delta
pkg:go.uber.org/yarpc/transport/grpc goos:darwin goarch:amd64
_ResponseWriter_AddHeaders/lowercase                             4.00 ± 0%      3.00 ± 0%   -25.00%  (p=0.000 n=10+10)
_ResponseWriter_AddHeaders/lowercase_no-value                    1.00 ± 0%      0.00       -100.00%  (p=0.000 n=10+10)
_ResponseWriter_AddHeaders/titlecase                             5.00 ± 0%      4.00 ± 0%   -20.00%  (p=0.000 n=10+10)
_ResponseWriter_AddHeaders/titlecase_no-value                    2.00 ± 0%      1.00 ± 0%   -50.00%  (p=0.000 n=10+10)
pkg:go.uber.org/yarpc/transport/http goos:darwin goarch:amd64
_ResponseWriter_AddHeaders/lowercase                             2.00 ± 0%      2.00 ± 0%      ~     (all equal)
_ResponseWriter_AddHeaders/titlecase                             3.00 ± 0%      2.00 ± 0%   -33.33%  (p=0.000 n=10+10)
_ResponseWriter_AddHeaders/titlecase_with_rpc-header_prefix      3.00 ± 0%      1.00 ± 0%   -66.67%  (p=0.000 n=10+10)
pkg:go.uber.org/yarpc/transport/tchannel goos:darwin goarch:amd64
_ResponseWriter_AddHeaders/lowercase                             0.00           0.00           ~     (all equal)
_ResponseWriter_AddHeaders/titlecase                             3.00 ± 0%      1.00 ± 0%   -66.67%  (p=0.000 n=10+10)

Benchmarks on AddHeader:

name                                                         time/op
pkg:go.uber.org/yarpc/transport/grpc goos:darwin goarch:amd64
_ResponseWriter_AddHeader/lowercase                          1.04µs ± 8%
_ResponseWriter_AddHeader/lowercase_no-value                 17.9ns ± 7%
_ResponseWriter_AddHeader/titlecase                          1.05µs ±10%
_ResponseWriter_AddHeader/titlecase_no-value                 39.3ns ±11%
pkg:go.uber.org/yarpc/transport/http goos:darwin goarch:amd64
_ResponseWriter_AddHeader/lowercase                           259ns ± 5%
_ResponseWriter_AddHeader/titlecase                           161ns ± 7%
_ResponseWriter_AddHeader/titlecase_with_rpc-header_prefix    114ns ±10%
pkg:go.uber.org/yarpc/transport/tchannel goos:darwin goarch:amd64
_ResponseWriter_AddHeader/lowercase                          47.6ns ±11%
_ResponseWriter_AddHeader/titlecase                           105ns ± 5%

name                                                         alloc/op
pkg:go.uber.org/yarpc/transport/grpc goos:darwin goarch:amd64
_ResponseWriter_AddHeader/lowercase                            232B ± 0%
_ResponseWriter_AddHeader/lowercase_no-value                  0.00B     
_ResponseWriter_AddHeader/titlecase                            247B ± 0%
_ResponseWriter_AddHeader/titlecase_no-value                  3.00B ± 0%
pkg:go.uber.org/yarpc/transport/http goos:darwin goarch:amd64
_ResponseWriter_AddHeader/lowercase                            121B ± 8%
_ResponseWriter_AddHeader/titlecase                            106B ± 8%
_ResponseWriter_AddHeader/titlecase_with_rpc-header_prefix    91.0B ±12%
pkg:go.uber.org/yarpc/transport/tchannel goos:darwin goarch:amd64
_ResponseWriter_AddHeader/lowercase                           0.00B     
_ResponseWriter_AddHeader/titlecase                           6.00B ± 0%

name                                                         allocs/op
pkg:go.uber.org/yarpc/transport/grpc goos:darwin goarch:amd64
_ResponseWriter_AddHeader/lowercase                            3.00 ± 0%
_ResponseWriter_AddHeader/lowercase_no-value                   0.00     
_ResponseWriter_AddHeader/titlecase                            4.00 ± 0%
_ResponseWriter_AddHeader/titlecase_no-value                   1.00 ± 0%
pkg:go.uber.org/yarpc/transport/http goos:darwin goarch:amd64
_ResponseWriter_AddHeader/lowercase                            2.00 ± 0%
_ResponseWriter_AddHeader/titlecase                            1.00 ± 0%
_ResponseWriter_AddHeader/titlecase_with_rpc-header_prefix     0.00     
pkg:go.uber.org/yarpc/transport/tchannel goos:darwin goarch:amd64
_ResponseWriter_AddHeader/lowercase                            0.00     
_ResponseWriter_AddHeader/titlecase                            2.00 ± 0%
CLAassistant commented 2 years ago

CLA assistant check
All committers have signed the CLA.

rabbbit commented 2 years ago

I know very little about yarpc, but - is the AddHeader a viable option?

Looking at https://github.com/yarpc/yarpc-go/blob/dev/api/transport/response.go#L55, we would need to modify the interface, which is a breaking change?

Sorry, I didn't call it out, I didn't realize the impact of it earlier.

gopalchouhan commented 2 years ago

I agree with @rabbbit responseWriter implements the interface transport.ResponseWriter, and transport.ResponseWrite interface has to be modified to make it work. Modifying an existing common interface could be a breaking change and is something needs to be planned very carefully while keeping all users in mind that include internal as well as external users. Also we should evaluate the value this change brings against the sensitivity of this change.

jstroem commented 2 years ago

I agree, the contribution of this PR is not too much and since it requires changing the interface it is not worth it.

I still think it makes sense to do the optimisations on picking the right header-key so you don't have to transport strings unnecessarily - so I'll open a new PR for that.

Closing this one.