[3/3 header changes][http] Emit metrics for new headers handling behaviour. Hide actual changes under feature flag. #2275
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## emit-metrics-for-new-behavior-step-1 #2275 +/- ##
========================================================================
- Coverage 85.33% 85.29% -0.05%
========================================================================
Files 271 271
Lines 15624 15648 +24
========================================================================
+ Hits 13333 13347 +14
- Misses 1872 1877 +5
- Partials 419 424 +5 ☔ View full report in Codecov by Sentry. |
transport/http/header.go
Outdated
| ) | ||
|
|
||
| // headerConverter converts HTTP headers to and from transport headers. | ||
| type headerMapper struct{ Prefix string } | ||
|
|
||
| var ( | ||
| applicationHeaders = headerMapper{ApplicationHeaderPrefix} | ||
|
|
||
| // enforceHeaderRules is a feature flag for a more strict error handling rules. |
There was a problem hiding this comment.
can you be more specific here? from the impl, it looks it means we will not attach headers if this value is set to true && isReservedHeaderPrefix returns true? If that is the case, please call it out in the comment
There was a problem hiding this comment.
Updated this comment
| @@ -95,7 +95,7 @@ func TestCreateRequest(t *testing.T) { | |||
|
|
|||
| for _, tt := range tests { | |||
| t.Run(tt.desc, func(t *testing.T) { | |||
| o := &Outbound{urlTemplate: defaultURLTemplate} | |||
| o := &Outbound{urlTemplate: defaultURLTemplate, transport: &Transport{}} | |||
There was a problem hiding this comment.
I added new field reservedHeaderMetric to transport and use it, so this test fails if transport is nil.
transport/http/handler.go
Outdated
| @@ -330,3 +340,9 @@ func getContentType(encoding transport.Encoding) string { | |||
| return "" | |||
| } | |||
| } | |||
|
|
|||
| func popHeader(h http.Header, n string) string { | |||
There was a problem hiding this comment.
move it back to the original place
| @@ -267,7 +277,7 @@ func (rw *responseWriter) Write(s []byte) (int, error) { | |||
| } | |||
|
|
|||
| func (rw *responseWriter) AddHeaders(h transport.Headers) { | |||
| applicationHeaders.ToHTTPHeaders(h, rw.w.Header()) | |||
| _, rw.err = applicationHeaders.ToHTTPHeaders(h, rw.w.Header(), rw.edgeMetrics) | |||
There was a problem hiding this comment.
should you use https://pkg.go.dev/go.uber.org/multierr to combine the errors here instead of override every time?
There was a problem hiding this comment.
AddHeaders is called only once, so that's why we can just set err
biosvs
force-pushed
the
emit-metrics-for-new-behavior-step-3
branch
from
4087147
to
cb2af3f
Compare
biosvs
force-pushed
the
emit-metrics-for-new-behavior-step-3
branch
from
cb2af3f
to
0c05bc3
Compare
Part of #2265