My first suspect for the failure may be related to this change, and I was actually looking at that;

• Old code would see if X-Forwarded-For header is set (and non-empty)
• But if there's no "comma" in the header, it would be just ignored
• If there was a comma, we would pick the first value, but ignore the following ones

This felt a bit odd (but maybe there was a specific reason for this); why ignore the Header if there's only 1 "X-Forwarded-For"?

edit: format is: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For

X-Forwarded-For: <client>, <proxy1>, <proxy2>

If a request goes through multiple proxies, the IP addresses of each successive proxy is listed. This means that, given well-behaved client and proxies, the rightmost IP address is the IP address of the most recent proxy and the leftmost IP address is the IP address of the originating client.

And the examples on that page (obviously, MDN is not the canonical source of truth) shows examples with only the <client> IP, so this should be valid?

But if there's no "comma" in the header, it would be just ignored

Are you sure about this? Maybe I'm missing something but https://pkg.go.dev/strings#Split

if s does not contain sep and sep is not empty, Split returns a slice of length 1 whose only element is s.

Oh, 🙈 you're right; guess I had a mental "off by one" moment?

so the length check was at least redundant ☺️

Since we already know (from line 194) that keyStr has "http.request." as its prefix, unconditionally slicing the string by a constant would be more optimal.

The compiler is smart enough to evaluate len(constant) at compile time.

the thing about slicing the strings like this always makes me feel uneasy because Go slices strings by bytes not by runes, so I'm happy with Trimming in this case because it avoids "bad habits"

Yes, I must admit I went for "readability" here.

Actually, we can completely drop the strings.HasPrefix(). strings.TrimPrefix() already handles that, and if there's no match in the switch, we'll continue with the fallback.

Changing that also makes the code easier to read, and we don't have to use the ugly goto fallback

diff --git a/context/http.go b/context/http.go
index 7471e572..7fb75790 100644
--- a/context/http.go
+++ b/context/http.go
@@ -191,10 +191,6 @@ func (ctx *httpRequestContext) Value(key interface{}) interface{} {
 			return ctx.r
 		}
 
-		if !strings.HasPrefix(keyStr, "http.request.") {
-			goto fallback
-		}
-
 		switch strings.TrimPrefix(keyStr, "http.request.") {
 		case "uri":
 			return ctx.r.RequestURI
@@ -220,10 +216,11 @@ func (ctx *httpRequestContext) Value(key interface{}) interface{} {
 			if ct != "" {
 				return ct
 			}
+		default:
+			// no match; fall back to standard behavior below
 		}
 	}
 
-fallback:
 	return ctx.Context.Value(key)
 }
 
@@ -296,10 +293,6 @@ func (irw *instrumentedResponseWriter) Value(key interface{}) interface{} {
 			return irw
 		}
 
-		if !strings.HasPrefix(keyStr, "http.response.") {
-			goto fallback
-		}
-
 		irw.mu.Lock()
 		defer irw.mu.Unlock()
 
@@ -313,9 +306,10 @@ func (irw *instrumentedResponseWriter) Value(key interface{}) interface{} {
 			if contentType != "" {
 				return contentType
 			}
+		default:
+			// no match; fall back to standard behavior below
 		}
 	}
 
-fallback:
 	return irw.Context.Value(key)
 }

Actually, we can completely drop the strings.HasPrefix(). strings.TrimPrefix() already handles that, and if there's no match in the switch, we'll continue with the fallback.

Not really: strings.TrimPrefix returns the input string unchanged if there is no match, so the switch would match both keyStr == "uri" and "http.request.uri" if we drop the strings.HasPrefix() check.

🤦 yup

Updated to use the full string in the switch (as we discussed) 👍

Aside: this should probably be changed to textproto.TrimString at some point, and the rest of the module audited for inappropriate uses of unicode functions in HTTP-related code.

Good one; TIL about that function (or at least, I don't recall I've used it before). Probably needs to be checked with punicode domains etc 🤔

This is technically a functional change: Version("").minor() will return strconv.ErrSyntax instead of panicking. I just want to make sure you are aware.

Let me check on that on in context (checking out the branch locally)

Ah, caught up; yes, indeed, previously it would panic. I'm assuming "panic" is not very desirable, so would consider it a bug fix? 😅

So, I suspect this may be a bug as well here (noticed this after your last comment); i.e. vars.FOO and FOO would be considered equal. This was already in place, and I'd have to check history, so I left this as a TODO for now.

Looked a bit at the code, and the Mutex appeared to be to protect the status and written fields, so I moved the locks to those specific cases. This allowed us to have a simple switch with all options, and prevents acquiring a lock where not needed (which could always be a risk).