[1/3 header changes][metrics + grpc] Emit metrics for new headers handling behaviour. Hide actual changes under feature flag. #2268

biosvs · 2024-04-25T15:31:13Z

Currently underlying implementation of headers handling wary significantly from one transport to another (docs/docs/headers-handling.md in this PR). Eventually, in one of the following releases, we want to make behaviour consistent and protective: filter inbound 'rpc-' headers, return error for attempting of 'rpc-' header setting.

Proposed changes are backward incompatible, so to identify the edges that are affected by the future changes, at first stage let's emit metrics.

Metrics: reserved_headers_stripped and reserved_headers_error metrics with "component": "yarpc-header-migration" constant tag and with source and dest variable tags

Part of #2265

codecov · 2024-04-25T15:45:37Z

Codecov Report

Attention: Patch coverage is 92.21557% with 13 lines in your changes missing coverage. Please review.

Project coverage is 85.29%. Comparing base (23c069f) to head (d8e451e).
Report is 5 commits behind head on dev.

Files	Patch %	Lines
transport/tchannel/response_writer.go	78.68%	7 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #2268      +/-   ##
==========================================
+ Coverage   85.23%   85.29%   +0.06%     
==========================================
  Files         270      272       +2     
  Lines       15555    15630      +75     
==========================================
+ Hits        13258    13332      +74     
+ Misses       1876     1875       -1     
- Partials      421      423       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AllenLuUber · 2024-04-30T02:15:52Z

docs/headers-handling.md

@@ -0,0 +1,86 @@
+# Headers handling
+
+Yarpc has unified API for getting and setting headers. Although implementations may wary 


fixing some typo and grammar: YARPC has a single unified API for getting and setting headers across 3 L7 protocols, although implementations may vary by a lot.

Also it is worth calling out HTTP and gRPC (a HTTP2 protocol) allows multiple key for the same key, which is why [go http|https://pkg.go.dev/net/http#Header] defines the type as map[string][]string. This is very different from TChannel.

One more thing you can call out is, HTTP/gRPC header is case insensitive, but TChannel header is case sensitive. This is why we have an OriginalHeaders on the yarpc.CallFromContext(ctx).OriginalHeaders method.

HTTP and gRPC support multiple values, but yarpc introduces undefined behaviour and uses only one value (https://github.com/yarpc/yarpc-go/blob/dev/transport/grpc/headers.go#L156, https://github.com/yarpc/yarpc-go/blob/dev/transport/http/header.go#L66).

Added these notes to doc.

AllenLuUber · 2024-04-30T02:26:26Z

docs/headers-handling.md

+
+### Inbound - Request (Parsing)
+
+Predefined list of headers (one header, actually) is read and stripped from the inbound request.


If it is just one, just phrase as: A single header (list the key) is read and stripped from the inbound request

AllenLuUber · 2024-04-30T02:27:55Z

docs/headers-handling.md

+
+Predefined list of headers (one header, actually) is read and stripped from the inbound request.
+
+All other headers are forwarded as is to an application code.


All other headers are forwarded as is to the application handler.

AllenLuUber · 2024-04-30T02:28:43Z

docs/headers-handling.md

+
+### Inbound - Response (Writing)
+
+Attempting to add a header with a name listed as reserved leads to an error "cannot use reserved header key".


ambiguity: reserved by tchannel or yarpc? Can you call out and add a link if possible

Added clarification and link

AllenLuUber · 2024-04-30T02:29:14Z

docs/headers-handling.md

+
+### Outbound - Response (Parsing)
+
+Headers with the names listed as reserved are deleted. All other headers are forwarded to an application code as is.


All other headers are forwarded to the outbound as is.

AllenLuUber · 2024-04-30T02:40:39Z

internal/observability/reserved_headers.go

+// IncStripped increments the stripped metric.
+func (m *ReservedHeaderMetrics) IncStripped(source, dest string) {
+	incHeaderMetric(m.strippedVec, source, dest)
+}
+
+// IncError increments the error metric.
+func (m *ReservedHeaderMetrics) IncError(source, dest string) {
+	incHeaderMetric(m.errorVec, source, dest)
+}


do we need those? If it is only used in IncStripped, you can just inline there?

func (r *ReservedHeaderReporter) IncStripped() { incHeaderMetric(r.m.strippedVec, source, dest) }

I checked, these calls are inlined by compiler, so we're fine in this regard. And as we're good from performance perspective, I would say, it's nice to have some ownership separation - edge doesn't know about internal structure of ReservedHeaderMetrics. Plus it may be useful one day anyway.

AllenLuUber · 2024-04-30T02:42:16Z

internal/observability/reserved_headers.go

+	return v
+}
+
+func incHeaderMetric(vector *metrics.CounterVector, source, dest string) {


incHeaderVecMetric?

AllenLuUber · 2024-04-30T03:06:49Z

transport/grpc/headers.go

@@ -153,9 +168,21 @@ func metadataToTransportRequest(md metadata.MD) (*transport.Request, error) {
 				request.Encoding = transport.Encoding(getContentSubtype(value))
 			}
 		default:
+			if isReservedHeaderPrefixV2(header) {
+				reportStrippedHeader = true


instead of a true/false value, do you want to use a counter here? Right now, no matter how many violations we have, we already inc by 1.

It won't change anything, +1 or +10 - it's basically gives no new information.

AllenLuUber · 2024-04-30T03:08:14Z

internal/observability/reserved_headers_test.go

+}
+
+func assertTuples(t *testing.T, snapshots []metrics.Snapshot, expected []tuple) {
+	var actual []tuple


nit: actual := make([]tuple, len(snapshots)*2)

AllenLuUber · 2024-04-30T03:09:26Z

internal/observability/reserved_headers_test.go

+		m := NewReserveHeaderMetrics(nil, "test")
+
+		m.IncStripped("", "")
+		m.IncError("", "")


should you assert something here? If you are testing no panic, you should do assert.NoPanic

…feature flag.

…ype and simplified interface

biosvs changed the title ~~[1/3] Emit metrics for new headers handling behaviour. Hide actual changes under feature flag.~~ [1/3 metrics + grpc] Emit metrics for new headers handling behaviour. Hide actual changes under feature flag. Apr 25, 2024

biosvs force-pushed the emit-metrics-for-new-behavior-step-1 branch from 2da1be9 to 61a74eb Compare April 25, 2024 15:42

AllenLuUber reviewed Apr 30, 2024

View reviewed changes

biosvs force-pushed the emit-metrics-for-new-behavior-step-1 branch from 61a74eb to 93a82ba Compare April 30, 2024 13:13

AllenLuUber approved these changes May 1, 2024

View reviewed changes

biosvs added 2 commits May 3, 2024 17:51

Define metrics for reserved headers reporting

06b2002

Emit metrics for reserved headers in GRPC. Hide actual changes under …

f7b3c8e

…feature flag.

biosvs force-pushed the emit-metrics-for-new-behavior-step-1 branch from 93a82ba to f7b3c8e Compare May 3, 2024 15:52

biosvs changed the title ~~[1/3 metrics + grpc] Emit metrics for new headers handling behaviour. Hide actual changes under feature flag.~~ [1/4 metrics + grpc] Emit metrics for new headers handling behaviour. Hide actual changes under feature flag. May 3, 2024

biosvs changed the title ~~[1/4 metrics + grpc] Emit metrics for new headers handling behaviour. Hide actual changes under feature flag.~~ [1/3 header changes][metrics + grpc] Emit metrics for new headers handling behaviour. Hide actual changes under feature flag. May 7, 2024

biosvs added 2 commits July 30, 2024 20:40

Extracted response writer to dedicated file, introduced constructor t…

1ef2dbf

…ype and simplified interface

Renamed headerCallerProcedureToRequest to transportHeadersToRequest

d8e451e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1/3 header changes][metrics + grpc] Emit metrics for new headers handling behaviour. Hide actual changes under feature flag. #2268

[1/3 header changes][metrics + grpc] Emit metrics for new headers handling behaviour. Hide actual changes under feature flag. #2268

biosvs commented Apr 25, 2024

codecov bot commented Apr 25, 2024 •

edited

Loading

AllenLuUber Apr 30, 2024

biosvs Apr 30, 2024

AllenLuUber Apr 30, 2024

biosvs Apr 30, 2024

AllenLuUber Apr 30, 2024

biosvs Apr 30, 2024

AllenLuUber Apr 30, 2024

biosvs Apr 30, 2024

AllenLuUber Apr 30, 2024

AllenLuUber Apr 30, 2024

biosvs Apr 30, 2024

AllenLuUber Apr 30, 2024

biosvs Apr 30, 2024

AllenLuUber Apr 30, 2024

biosvs Apr 30, 2024

AllenLuUber Apr 30, 2024

biosvs Apr 30, 2024

AllenLuUber Apr 30, 2024

biosvs Apr 30, 2024

		@@ -0,0 +1,86 @@
		# Headers handling

		Yarpc has unified API for getting and setting headers. Although implementations may wary


		### Inbound - Request (Parsing)

		Predefined list of headers (one header, actually) is read and stripped from the inbound request.


		### Inbound - Response (Writing)

		Attempting to add a header with a name listed as reserved leads to an error "cannot use reserved header key".


		### Outbound - Response (Parsing)

		Headers with the names listed as reserved are deleted. All other headers are forwarded to an application code as is.

[1/3 header changes][metrics + grpc] Emit metrics for new headers handling behaviour. Hide actual changes under feature flag. #2268

Are you sure you want to change the base?

[1/3 header changes][metrics + grpc] Emit metrics for new headers handling behaviour. Hide actual changes under feature flag. #2268

Conversation

biosvs commented Apr 25, 2024

codecov bot commented Apr 25, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 25, 2024 •

edited

Loading