add initial version of W3C tracestate #887

reyang · 2018-08-30T19:01:28Z

This is the initial implementation of W3C tracestate in Go.

This is my first time writing program in Go, couple notes/questions:

I want to discuss if want to put thread safety guard on Tracestate.Fork(), or leave it as a responsibility for the API consumer.
Currently the Tracestate object will be copied every time when a new span got created, I plan to implement COW (copy on write) to improve perf.
My next pull request will implement the ochttp integration, which will propagate tracestate in HTTP headers.

reyang · 2018-08-30T23:48:36Z

@rakyll @Ramonza, would you give me some hints? I'm getting the following AppVeyor build error which claims gcc compiler missing:

runtime/cgo
exec: "gcc": executable file not found in %PATH%
FAIL go.opencensus.io/exporter/jaeger [build failed]
FAIL go.opencensus.io/exporter/prometheus [build failed]
FAIL go.opencensus.io/exporter/stackdriver/propagation [build failed]
FAIL go.opencensus.io/exporter/zipkin [build failed]
FAIL go.opencensus.io/internal [build failed]
FAIL go.opencensus.io/plugin/ocgrpc [build failed]
FAIL go.opencensus.io/plugin/ochttp [build failed]
FAIL go.opencensus.io/plugin/ochttp/propagation/b3 [build failed]
FAIL go.opencensus.io/plugin/ochttp/propagation/tracecontext [build failed]
FAIL go.opencensus.io/stats [build failed]
FAIL go.opencensus.io/stats/view [build failed]
FAIL go.opencensus.io/tag [build failed]
FAIL go.opencensus.io/trace [build failed]
FAIL go.opencensus.io/trace/propagation [build failed]
FAIL go.opencensus.io/zpages [build failed]
Command exited with code 2

semistrict · 2018-08-31T16:34:01Z

trace/tracestate.go

+// TraceState is a tracing-system specific context in a list of key-value pairs.
+// TraceState allows different vendors propagate additional information and
+// inter-operate with their legacy Id formats.
+type Tracestate struct {


Why is this not just a map?

because order of key-value pairs is important, see:
https://w3c.github.io/distributed-tracing/trace_context/HTTP_HEADER_FORMAT_RATIONALE.html#ordering-of-keys-in-tracestate

@Ramonza - @basvanbeek is right, the W3C spec requires tracestate to following a special ordering rule - last modified key-value-pair should appear at the very beginning.

@reyang I think with @Ramonza questioning the choice I think it makes sense to add a comment in the code so it is clear to everybody without needing to deep dive into the specs.

semistrict · 2018-08-31T16:34:20Z

trace/tracestate.go

+	VALUE_FORMAT              = `[\x20-\x2b\x2d-\x3c\x3e-\x7e]{0,255}[\x21-\x2b\x2d-\x3c\x3e-\x7e]`
+)
+
+var KEY_VALIDATION_RE = regexp.MustCompile(`^(` + KEY_FORMAT + `)$`)


Go convention for variables is camelCase

Got it, I'm fixing it.

semistrict · 2018-08-31T16:34:37Z

trace/tracestate.go

+)
+
+const (
+	KEY_WITHOUT_VENDOR_FORMAT = `[a-z][_0-9a-z\-\*\/]{0,255}`


Go convention for consts is camelCase - I think golint will pick this up

Sure, I'd be glad to change to camelCase.

I did searched the entire repo to see if there is a convention I could follow, and I'm seeing inconsistent conventions. And golint is not part of CI validation (all I see is gofmt). e.g.

In stats/validation.go, we have

const ( MaxNameLength = 255 )

in vendor\git.apache.org\thrift.git\lib\go\thrift\application_exception.go (although this is 3rd party dependency), we have

const ( UNKNOWN_APPLICATION_EXCEPTION = 0 UNKNOWN_METHOD = 1 INVALID_MESSAGE_TYPE_EXCEPTION = 2 WRONG_METHOD_NAME = 3 BAD_SEQUENCE_ID = 4 MISSING_RESULT = 5 INTERNAL_ERROR = 6 PROTOCOL_ERROR = 7 )

semistrict · 2018-08-31T16:35:34Z

trace/trace.go

@@ -289,6 +295,14 @@ func (s *Span) SpanContext() SpanContext {
 	return s.spanContext
 }

+// Tracestate returns the Tracestate of the span context.
+func (s *Span) Tracestate() *Tracestate {


Why isn't this a method on SpanContext?

@Ramonza, I'll move this to SpanContext.

semistrict · 2018-08-31T16:37:42Z

trace/tracestate.go

+	return ts.Set(key, "")
+}
+
+func (ts *Tracestate) Set(key string, value string) string {


I don't think we should make any part of SpanContext mutable. Can we make Tracestate immutable for now? I think we can figure out how to make it possible to change the Tracestate in a subsequent PR - I think the focus of this initial work should just be on correct propagation.

Sure, we can take the step-by-step approach.

To provide more context, census-instrumentation/opencensus-specs#70 (comment) and census-instrumentation/opencensus-specs#131.

Also adding @wu-sheng for awareness.

@Ramonza, I've commented out the Tracestate.Remove and Tracestate.Set for now.

Regarding SpanContext, I'm putting a pointer to Tracestate rather than a hardy copy for Tracestate as we would want to have COW (copy-on-write) Tracestate in the future for performance, taking a hard copy is too expensive.

Would you mind removing them entirely for now? We in general never keep around commented code.

Sure, I'll remove it.

semistrict · 2018-08-31T16:37:52Z

trace/tracestate.go

+		VALUE_VALIDATION_RE.MatchString(ts.value)
+}
+
+// TraceState is a tracing-system specific context in a list of key-value pairs.


Inconsistent capitalization

semistrict

Some further comments. I think we need more test coverage. I would also ideally like to see somewhere where we actually propagate Tracestate. It's most important that we correctly propagate it for now I think so that we don't break the chain of propagation.

semistrict · 2018-08-31T21:41:46Z

trace/tracestate.go

+	return ts.Set(key, "")
+}
+
+func (ts *Tracestate) Set(key string, value string) string {


Would you mind removing them entirely for now? We in general never keep around commented code.

semistrict · 2018-08-31T21:42:49Z

trace/tracestate.go

+	return true
+}
+
+func (ts *Tracestate) Fork() *Tracestate {


I think we should remove this method as well. It only makes sense if Tracestate is mutable.

semistrict · 2018-08-31T21:44:26Z

trace/tracestate.go

+*/
+
+func (ts *Tracestate) String() string {
+	return fmt.Sprintf("tracestate%s", ts.entries)


I don't think this will be a useful output for String() - I would recommend just removing the method

Alternatively, I think it would be reasonable for String() to produce a Tracestate header value in the format from the spec.

Please refer to census-instrumentation/opencensus-python#238 (comment) where we agreed to separate the in-process representation and the actual HTTP header format.

semistrict · 2018-08-31T21:45:08Z

trace/trace_test.go

@@ -510,6 +520,7 @@ func TestSetSpanStatus(t *testing.T) {
 			TraceID:      tid,
 			SpanID:       SpanID{},
 			TraceOptions: 0x1,
+			Tracestate:   &Tracestate{},


The zero value of *Tracestate (nil) should be a valid empty Tracestate in the spirit of making the zero value useful. So it should not be necessary to set it here.

semistrict · 2018-08-31T21:46:07Z

trace/tracestate.go

+}
+
+func (ts *Tracestate) Get(key string) string {
+	for _, entry := range ts.entries {


To make the zero value of *Tracestate useful, let's add

if ts == nil { return "" }

Should this method distinguish between an empty string value and the absence of a key? If so, perhaps it should return (string, bool)?

Empty string means the key doesn't exist, as the W3C spec wouldn't allow empty string as the value.

semistrict · 2018-08-31T21:47:31Z

trace/tracestate.go

+}
+
+func (ts *TracestateEntry) IsValid() bool {
+	return keyValidationRegExp.MatchString(ts.key) &&


Is an empty key valid? Is an empty value? It seems from the pattern that it is. Should it be? Ideally we would have a test for this.

reyang · 2018-08-31T21:52:50Z

@Ramonza, agree, that's what I've mentioned in the PR description "My next pull request will implement the ochttp integration, which will propagate tracestate in HTTP headers.", I will add the actual propagation and the test cases in the next PR.

semistrict · 2018-08-31T21:54:07Z

I would prefer to have it in one PR because I don't think just this alone makes sense on its own -- it doesn't add any functionality and I'm worried that if users see it they will be confused as to why its there.

semistrict · 2018-08-31T21:54:22Z

I am going to look into the AppVeyor failure, it is unrelated.

reyang · 2018-08-31T22:38:21Z

Close this as it is a dup with #890 which @rghetia is working on.

reyang added 3 commits August 30, 2018 11:53

add initial version of W3C tracestate

581cd79

gofmt

163de2f

accept nil tracestate in parent span context

4e23146

semistrict reviewed Aug 31, 2018

View reviewed changes

reyang added 5 commits August 31, 2018 10:15

resolve review comments

9329855

fix gofmt error

56ff1e2

make tracestate immutable for now

727300c

fix typo

e3bec5b

use Span.SpanContext().Tracestate instead of Span.Tracestate()

f87c2a7

semistrict reviewed Aug 31, 2018

View reviewed changes

rghetia mentioned this pull request Aug 31, 2018

Add Tracestate to SpanContext #890

Merged

reyang closed this Aug 31, 2018

reyang deleted the tracestate branch September 25, 2018 22:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add initial version of W3C tracestate #887

add initial version of W3C tracestate #887

reyang commented Aug 30, 2018

reyang commented Aug 30, 2018

semistrict Aug 31, 2018

basvanbeek Aug 31, 2018

reyang Aug 31, 2018

basvanbeek Aug 31, 2018

semistrict Aug 31, 2018

reyang Aug 31, 2018

semistrict Aug 31, 2018

reyang Aug 31, 2018

semistrict Aug 31, 2018

reyang Aug 31, 2018 •

edited

Loading

semistrict Aug 31, 2018

reyang Aug 31, 2018

reyang Aug 31, 2018

semistrict Aug 31, 2018

reyang Aug 31, 2018

semistrict Aug 31, 2018

reyang Aug 31, 2018

semistrict left a comment

semistrict Aug 31, 2018

semistrict Aug 31, 2018

semistrict Aug 31, 2018

semistrict Aug 31, 2018 •

edited

Loading

reyang Aug 31, 2018

semistrict Aug 31, 2018 •

edited

Loading

semistrict Aug 31, 2018 •

edited

Loading

semistrict Aug 31, 2018

reyang Aug 31, 2018

semistrict Aug 31, 2018

reyang commented Aug 31, 2018

semistrict commented Aug 31, 2018

semistrict commented Aug 31, 2018

reyang commented Aug 31, 2018

add initial version of W3C tracestate #887

add initial version of W3C tracestate #887

Conversation

reyang commented Aug 30, 2018

reyang commented Aug 30, 2018

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

reyang Aug 31, 2018 • edited Loading

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

semistrict left a 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

semistrict Aug 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

semistrict Aug 31, 2018 • edited Loading

Choose a reason for hiding this comment

semistrict Aug 31, 2018 • edited Loading

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

reyang commented Aug 31, 2018

semistrict commented Aug 31, 2018

semistrict commented Aug 31, 2018

reyang commented Aug 31, 2018

reyang Aug 31, 2018 •

edited

Loading

semistrict Aug 31, 2018 •

edited

Loading

semistrict Aug 31, 2018 •

edited

Loading

semistrict Aug 31, 2018 •

edited

Loading