do not discard the Instana tracestate list member

Previously, when more than 32 list members were in the incoming
tracestate header value, and the Instana-specific list member (in=...)
was at position 33 or later, we would drop it together with the other
list members that need to be droppped. With this change, we give the
Instana list member higher priority than list members from other
vendors.

We also keep it separate in the TraceState struct.

Furthermore, this simplifies/reduces the API of the TraceState module.
We do not have a use case for adding/removing list members for arbitrary
vendors - we only need to propagate the list members we received and
make sure that the Instana trace state value is added in the left-most
position.

Author: Bastian Krol

propagation.go

@@ -234,7 +234,7 @@ func addW3CTraceContext(h http.Header, sc SpanContext) {
 	// participate in w3c trace context if tracing is enabled
 	if !sc.Suppressed {
 		// propagate truncated trace ID downstream
-		trCtx.RawState = trCtx.State().Add(w3ctrace.VendorInstana, FormatID(sc.TraceID)+";"+spanID).String()
+		trCtx.RawState = trCtx.State().SetInstanaTraceStateValue(FormatID(sc.TraceID)+";"+spanID).String()
 	}
 
 	w3ctrace.Inject(trCtx, h)

span_context.go

@@ -103,7 +103,7 @@ func NewSpanContext(parent SpanContext) SpanContext {
 	// check if there is Instana state stored in the W3C tracestate header
 	if foreignTrace {
 		w3cState := c.W3CContext.State()
-		if ancestor, ok := w3cState.Fetch(w3ctrace.VendorInstana); ok {
+		if ancestor, ok := w3cState.FetchInstanaTraceStateValue(); ok {
 			if ref, ok := parseW3CInstanaState(ancestor); ok {
 				c.Links = append(c.Links, ref)
 			}
@@ -160,7 +160,7 @@ func restoreFromW3CTraceState(trCtx w3ctrace.Context) SpanContext {
 		W3CContext: trCtx,
 	}
 
-	state, ok := trCtx.State().Fetch(w3ctrace.VendorInstana)
+	state, ok := trCtx.State().FetchInstanaTraceStateValue()
 	if !ok {
 		return c
 	}

w3ctrace/context_test.go

@@ -95,7 +95,7 @@ func TestContext_State(t *testing.T) {
 		RawState:  exampleTraceState,
 	}
 
-	assert.Equal(t, w3ctrace.State{"vendorname1=opaqueValue1 ", " vendorname2=opaqueValue2"}, trCtx.State())
+	assert.Equal(t, w3ctrace.NewState([]string{"vendorname1=opaqueValue1 ", " vendorname2=opaqueValue2"}, ""), trCtx.State())
 }
 
 func TestContext_Parent(t *testing.T) {

w3ctrace/state.go

@@ -5,6 +5,7 @@ package w3ctrace
 
 import (
 	"bytes"
+	"regexp"
 	"strings"
 )
 
@@ -17,109 +18,117 @@ const maxKVPairs = 32
 // Length of entries that should be filtered first in case, if tracestate has more than `maxKVPairs` items
 const thresholdLen = 128
 
-// State is list of key=value pairs representing vendor-specific data in the trace context
-type State []string
+var instanaListMemberRegex = regexp.MustCompile("^\\s*" + VendorInstana + "\\s*=\\s*([^,]*)\\s*$")
 
-// ParseState parses the value of `tracestate` header. Empty list items are omitted.
-func ParseState(traceStateValue string) (State, error) {
-	states := filterEmptyItems(strings.Split(traceStateValue, ","))
-	if len(states) < maxKVPairs {
-		return states, nil
-	}
+// State is list of key=value pairs representing vendor-specific data in the trace context
+type State struct {
+	// The key-value pairs of other vendors. The "in" list member, if present, will be stored separately
+	// (Note: list member is the term the W3C trace context specfication uses for the key-value pairs.)
+	listMembers []string
 
-	itemsToFilter := len(states) - maxKVPairs
-	filteredStates := states[:0]
+	// The value for the Instana-specific list member ("in=...").
+	instanaTraceStateValue string
+}
 
-	i := 0
-	for ; itemsToFilter > 0 && i < len(states); i++ {
+// makeState creates a new State with the given values
+func NewState(listMembers []string, instanaTraceStateValue string) State {
+	return State{
+		listMembers:            listMembers,
+		instanaTraceStateValue: instanaTraceStateValue,
+	}
+}
 
-		if len(states[i]) > thresholdLen {
-			itemsToFilter--
-			continue
+// ParseState parses the value of `tracestate` header. Empty list items are omitted.
+func ParseState(traceStateValue string) (State, error) {
+	listMembers := filterEmptyItems(strings.Split(traceStateValue, ","))
+
+	// Look for the Instana list member first, before discarding any list members due to length restrictions.
+	var instanaTraceStateValue string
+	instanaTraceStateIdx := -1
+	for i, vd := range listMembers {
+		matchResult := instanaListMemberRegex.FindStringSubmatch(vd)
+		if len(matchResult) == 2 {
+			instanaTraceStateValue = strings.TrimSpace(matchResult[1])
+			instanaTraceStateIdx = i
+			break
 		}
-
-		filteredStates = append(filteredStates, states[i])
 	}
-	filteredStates = append(filteredStates, states[i:]...)
 
-	if len(filteredStates) > maxKVPairs {
-		return filteredStates[:maxKVPairs], nil
+	if instanaTraceStateIdx >= 0 {
+		// remove the entry for instana from the array of list members
+		listMembers = append(listMembers[:instanaTraceStateIdx], listMembers[instanaTraceStateIdx+1:]...)
 	}
 
-	return filteredStates, nil
-}
-
-// Add returns a new state prepended with provided vendor-specific data. It removes any existing
-// entries for this vendor and returns the same state if vendor is empty. If the number of entries
-// in a state reaches the MaxStateEntries, rest of the items will be truncated
-func (st State) Add(vendor, data string) State {
-	if vendor == "" {
-		return st
+	// Depending on whether we found an Instana list member, we can either allow 31 or 32 list members from other vendors.
+	maxListMembers := maxKVPairs
+	if instanaTraceStateValue != "" {
+		maxListMembers--
 	}
 
-	newSt := make(State, 1, len(st)+1)
-	newSt[0] = vendor + "=" + data
-	newSt = append(newSt, st.Remove(vendor)...)
-
-	// truncate the state if it reached the max number of entries
-	if len(newSt) > MaxStateEntries {
-		newSt = newSt[:MaxStateEntries]
+	if len(listMembers) < maxListMembers {
+		return State{listMembers: listMembers, instanaTraceStateValue: instanaTraceStateValue}, nil
 	}
 
-	return newSt
-}
-
-// Fetch retrieves stored vendor-specific data for given vendor
-func (st State) Fetch(vendor string) (string, bool) {
-	i := st.Index(vendor)
-	if i < 0 {
-		return "", false
+	itemsToFilter := len(listMembers) - maxListMembers
+	filteredListMembers := listMembers[:0]
+	i := 0
+	for ; itemsToFilter > 0 && i < len(listMembers); i++ {
+		if len(listMembers[i]) > thresholdLen {
+			itemsToFilter--
+			continue
+		}
+		filteredListMembers = append(filteredListMembers, listMembers[i])
 	}
+	filteredListMembers = append(filteredListMembers, listMembers[i:]...)
 
-	return strings.TrimPrefix(st[i], vendor+"="), true
-}
-
-// Index returns the index of vendor-specific data for given vendor in the state.
-// It returns -1 if the state does not contain data for this vendor.
-func (st State) Index(vendor string) int {
-	prefix := vendor + "="
-
-	for i, vd := range st {
-		if strings.HasPrefix(vd, prefix) {
-			return i
-		}
+	if len(filteredListMembers) > maxListMembers {
+		return State{listMembers: filteredListMembers[:maxListMembers], instanaTraceStateValue: instanaTraceStateValue}, nil
 	}
 
-	return -1
+	return State{listMembers: filteredListMembers, instanaTraceStateValue: instanaTraceStateValue}, nil
 }
 
-// Remove returns a new state without data for specified vendor. It returns the same state if vendor is empty
-func (st State) Remove(vendor string) State {
-	if vendor == "" {
-		return st
+// SetInstanaValue returns a new state prepended with the provided Instana value. If the original state had an Instana
+// list member pair, it is discarded/overwritten.
+func (st State) SetInstanaTraceStateValue(instanaTraceStateValue string) State {
+	var listMembers []string
+	if st.instanaTraceStateValue == "" && instanaTraceStateValue != "" && len(st.listMembers) == maxKVPairs {
+		// The incoming tracestate had the maximum number of list members but no Instana list member, now we are adding an
+		// Instana list member, so we would exceed the maximum by one. Hence, we need to discard one of the other list
+		// members to stay within the limits mandated by the specification.
+		listMembers = st.listMembers[:maxKVPairs-1]
+	} else {
+		listMembers = st.listMembers
 	}
 
-	prefix := vendor + "="
-
-	var newSt State
-	for _, vd := range st {
-		if !strings.HasPrefix(vd, prefix) {
-			newSt = append(newSt, vd)
-		}
-	}
+	return State{listMembers: listMembers, instanaTraceStateValue: instanaTraceStateValue}
+}
 
-	return newSt
+// FetchInstanaTraceStateValue retrieves the value of the Instana tracestate list member, if any.
+func (st State) FetchInstanaTraceStateValue() (string, bool) {
+	return st.instanaTraceStateValue, st.instanaTraceStateValue != ""
 }
 
 // String returns string representation of a trace state. The returned value is compatible with the
-// `tracestate` header format
+// `tracestate` header format. If the state has an Instana-specific list member, that one is always rendered first. This
+// is optimized for the use case of injecting the string represenation of the tracestate header into downstream
+// requests.
 func (st State) String() string {
-	if len(st) == 0 {
+	if len(st.listMembers) == 0 && st.instanaTraceStateValue == "" {
 		return ""
 	}
+	if len(st.listMembers) == 0 {
+		return VendorInstana + "=" + st.instanaTraceStateValue
+	}
 
 	buf := bytes.NewBuffer(nil)
-	for _, vd := range st {
+	if st.instanaTraceStateValue != "" {
+		buf.WriteString(VendorInstana)
+		buf.WriteString("=")
+		buf.WriteString(st.instanaTraceStateValue)
+		buf.WriteString(",")
+	}
+	for _, vd := range st.listMembers {
 		buf.WriteString(vd)
 		buf.WriteByte(',')
 	}