trimming space in parseCommaSeparatedMap

[jsongHBO]

The comments in https://github.com/jaegertracing/jaeger-client-go/blob/master/propagation.go#L267-L272
suggest that if I enter "key3 = value3 ", then it should return map[key3:value3]

But it doesn't
https://goplay.space/#K-zW71Vr33C

package main

import (
	"fmt"
	"log"
	"net/url"
	"strings"
)

func parseCommaSeparatedMap(value string) map[string]string {
	baggage := make(map[string]string)
	value, err := url.QueryUnescape(value)
	if err != nil {
		log.Printf("Unable to unescape %s, %v", value, err)
		return baggage
	}
	for _, kvpair := range strings.Split(value, ",") {
		kv := strings.Split(strings.TrimSpace(kvpair), "=")
		if len(kv) == 2 {
			baggage[kv[0]] = kv[1]
		} else {
			log.Printf("Malformed value passed in for %s", "testHeader")
		}
	}
	return baggage
}

func main() {
	b := parseCommaSeparatedMap("key2=value2,    key3    =    value3   ")
	fmt.Printf("%+v", b)
	// Returns map[key2:value2 key3    :    value3]
}

since trimming is not done after the = separation.
I'm not sure if this is intentional or a bug. If it's intentional, then the comments should be updated.

[black-adder]

Thanks for catching this; it looks like a bug

[jsongHBO]

Related Issue in JS jaegertracing/jaeger-client-node#244

[dcarney]

@jsongHBO @black-adder I have a PR coming right up! Thanks for opening this issue; I'll reference it in my PR.

[dcarney]

@black-adder Actually, there's an existing test case that looks like it's expecting that the whitespace is preserved:

{"hobbit=Bilbo Baggins, dwarf= Thrain", map[string]string{"hobbit": "Bilbo Baggins", "dwarf": " Thrain"}},

I think it's counter-intuitive (not to mention inconsistent with the associated godoc comments) to preserve whitespace in this way, but maybe this was done intentionally for some reason?

I'll go ahead with my PR, and we can hash it out there.

[black-adder]

@yurishkuro is this intended behavior? Seems weird that we trim the whitespace before the key and after the value but not in between.

[dcarney]

Opened #272 to address this, if indeed is a bug and not some intended behavior.

[yurishkuro]

we have plans to switch baggage propagation format from uberctx-{key}: value to jaeger-baggage: key=value. When we do this, I would expect the following to hold

span.SetBaggage("key", " value")
// --- in another process
span.GetBaggage("key") == " value" // leading space is preserved

If we always trim the spaces the above will be broken. One could argue (correctly) that when encoding baggage in the client process it should use url encoding for the value, i.e. key=%20value, however that doesn't help with TEXT_MAP carriers that do not require any encoding.

[jsongHBO]

What about the key? Should key also be encoded key%20%20 or simply preserving the space key____ ?

[yurishkuro]

keys can be trimmed since we do not support spaces in them anyway with the current propagation format

[lastchiliarch]

keys can be trimmed since we do not support spaces in them anyway with the current propagation format

I'd like to fix it.