Repository: vitessio/vitess
Pull request #6463 (2020-07-22 06:42:34)

change how tracing query comments are handled
When sending the trace information over the mysql protocol, we need to be able to encode a string-string map into a string that can be put into a query as a comment. The current way of doing it is broken, since it is not protecting the map serialisation from clashing with SQL.

This PR changes the approach to use first JSON as a way of encoding the map, and then base64 encoding to make sure that the json doesn't clash with any SQL syntax rules.

On the client side, one would need to pick up the current tracing information from openTracing, encode it as JSON, base64 encode the json-string, and stick this into the comment. Example:

```
openTracingData := map[string]string{
	"uber-trace-id":"123:456:789:1",
	"other data":   "something else",
}

jsonBytes, _ := json.Marshal(openTracingData)
encodedComment := base64.StdEncoding.EncodeToString(jsonBytes)

originalQuery := "select 42"

fullQuery := fmt.Sprintf("/*VT_SPAN_CONTEXT=%s*/ %s", encodedComment, originalQuery)

fmt.Println(fullQuery)
```
Result:
```
/*VT_SPAN_CONTEXT=eyJvdGhlciBkYXRhIjoic29tZXRoaW5nIGVsc2UiLCJ1YmVyLXRyYWNlLWlkIjoiMTIzOjQ1Njo3ODk6MSJ9*/ select 42
```

This PR replaces #6387 



Patch:
diff --git a/go/trace/opentracing.go b/go/trace/opentracing.go
index 6e6c6c5bc4..3a38b2647c 100644
--- a/go/trace/opentracing.go
+++ b/go/trace/opentracing.go
@@ -17,13 +17,13 @@ limitations under the License.
 package trace
 
 import (
-	"strings"
+	"encoding/base64"
+	"encoding/json"
 
 	otgrpc "github.com/opentracing-contrib/go-grpc"
 	"github.com/opentracing/opentracing-go"
 	"golang.org/x/net/context"
 	"google.golang.org/grpc"
-	"vitess.io/vitess/go/vt/proto/vtrpc"
 	"vitess.io/vitess/go/vt/vterrors"
 )
 
@@ -86,19 +86,18 @@ func (jf openTracingService) New(parent Span, label string) Span {
 }
 
 func extractMapFromString(in string) (opentracing.TextMapCarrier, error) {
-	m := make(opentracing.TextMapCarrier)
-	items := strings.Split(in, ":")
-	if len(items) < 2 {
-		return nil, vterrors.Errorf(vtrpc.Code_INVALID_ARGUMENT, "expected transmitted context to contain at least span id and trace id")
+	decodedBytes, err := base64.StdEncoding.DecodeString(in)
+	if err != nil {
+		return nil, err
 	}
-	for _, v := range items {
-		idx := strings.Index(v, "=")
-		if idx < 1 {
-			return nil, vterrors.Errorf(vtrpc.Code_INVALID_ARGUMENT, "every element in the context string has to be in the form key=value")
-		}
-		m[v[0:idx]] = v[idx+1:]
+
+	var dat opentracing.TextMapCarrier
+	err = json.Unmarshal(decodedBytes, &dat)
+	if err != nil {
+		return nil, err
 	}
-	return m, nil
+
+	return dat, nil
 }
 
 func (jf openTracingService) NewFromString(parent, label string) (Span, error) {


Test patch:
diff --git a/go/trace/opentracing_test.go b/go/trace/opentracing_test.go
index 19bdbce901..104545fe65 100644
--- a/go/trace/opentracing_test.go
+++ b/go/trace/opentracing_test.go
@@ -17,6 +17,8 @@ limitations under the License.
 package trace
 
 import (
+	"encoding/base64"
+	"encoding/json"
 	"testing"
 
 	"github.com/opentracing/opentracing-go"
@@ -25,17 +27,23 @@ import (
 
 func TestExtractMapFromString(t *testing.T) {
 	expected := make(opentracing.TextMapCarrier)
-	expected["apa"] = "12"
-	expected["banan"] = "x-tracing-backend-12"
-	result, err := extractMapFromString("apa=12:banan=x-tracing-backend-12")
+	expected["uber-trace-id"] = "123:456:789:1"
+	expected["other data with weird symbols:!#;"] = ":1!\""
+	jsonBytes, err := json.Marshal(expected)
+	assert.NoError(t, err)
+
+	encodedString := base64.StdEncoding.EncodeToString(jsonBytes)
+
+	result, err := extractMapFromString(encodedString)
 	assert.NoError(t, err)
 	assert.Equal(t, expected, result)
 }
 
 func TestErrorConditions(t *testing.T) {
-	_, err := extractMapFromString("")
+	encodedString := base64.StdEncoding.EncodeToString([]byte(`{"key":42}`))
+	_, err := extractMapFromString(encodedString) // malformed json {"key":42}
 	assert.Error(t, err)
 
-	_, err = extractMapFromString("key=value:keywithnovalue")
+	_, err = extractMapFromString("this is not base64") // malformed base64
 	assert.Error(t, err)
 }