[2/3 header changes][tchannel] Emit metrics for new headers handling behaviour. Hide actual changes under feature flag. #2272
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## tchannel-refactoring-2 #2272 +/- ##
==========================================================
+ Coverage 85.31% 85.34% +0.03%
==========================================================
Files 272 272
Lines 15630 15669 +39
==========================================================
+ Hits 13334 13373 +39
Misses 1874 1874
Partials 422 422 ☔ View full report in Codecov by Sentry. |
biosvs
changed the title
biosvs
force-pushed
the
emit-metrics-for-new-behavior-step-2
branch
from
1037c9f
to
5f291fa
Compare
biosvs
changed the base branch from
emit-metrics-for-new-behavior-step-1-2
to
tchannel-refactoring-2
biosvs
force-pushed
the
emit-metrics-for-new-behavior-step-2
branch
from
5f291fa
to
09b3d5f
Compare
| @@ -195,7 +195,9 @@ func (o *ChannelOutbound) Call(ctx context.Context, req *transport.Request) (*tr | |||
| } | |||
|
|
|||
| err = getResponseError(headers) | |||
| deleteReservedHeaders(headers) | |||
| // no check: err will be returned as is | |||
There was a problem hiding this comment.
is this comment intentional?
There was a problem hiding this comment.
Yeah, for some reason I was asking myself "where's error handling?" every time I look at this line, so I decided to left a comment
transport/tchannel/header.go
Outdated
|
|
||
| func deleteReservedPrefixHeaders(headers transport.Headers, edgeMetrics observability.ReservedHeaderEdgeMetrics) { | ||
| for key := range headers.Items() { | ||
| if isReservedHeaderPrefix(key) { |
There was a problem hiding this comment.
let's reduce some nesting here:
if !isReservedHeaderPrefix(key) {
continue
}
edgeMetrics.IncStripped()
if enforceHeaderRules {
headers.Del(key)
}another question, I don't see reuse here. Can we just combine those 2 and make a single deleteReservedHeaders func?
There was a problem hiding this comment.
Fixed
another question, I don't see reuse here
It's reused here: https://github.com/yarpc/yarpc-go/pull/2272/files#diff-72c944f1bd46d354d3c1255093274ae1ec4fca8fef63abdcd2fbc0f6ee738e86R188
| if isReservedHeaderKey(k) { | ||
| w.failedWith = appendError(w.failedWith, fmt.Errorf("cannot use reserved header key: %s", k)) | ||
| return | ||
| } | ||
|
|
||
| // Header with reserved prefix is used, but it's not a reserved key (existing rule), |
There was a problem hiding this comment.
remove existing rule? after this change is merged, we don't know what it means?
…der feature flag.
biosvs
force-pushed
the
emit-metrics-for-new-behavior-step-2
branch
from
09b3d5f
to
38ecd35
Compare
Part of #2265