Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

add initial version of W3C tracestate #887

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions trace/trace.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ type SpanContext struct {
TraceID TraceID
SpanID SpanID
TraceOptions TraceOptions
Tracestate *Tracestate
}

type contextKey struct{}
Expand Down Expand Up @@ -193,6 +194,11 @@ func startSpanInternal(name string, hasParent bool, parent SpanContext, remotePa
if !hasParent {
span.spanContext.TraceID = cfg.IDGenerator.NewTraceID()
}
if hasParent && parent.Tracestate != nil {
span.spanContext.Tracestate = parent.Tracestate.Fork()
} else {
span.spanContext.Tracestate = &Tracestate{}
}
span.spanContext.SpanID = cfg.IDGenerator.NewSpanID()
sampler := cfg.DefaultSampler

Expand Down
12 changes: 12 additions & 0 deletions trace/trace_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ func TestSampling(t *testing.T) {
TraceID: tid,
SpanID: sid,
TraceOptions: test.parentTraceOptions,
Tracestate: &Tracestate{},
}
ctx, _ = StartSpanWithRemoteParent(context.Background(), "foo", sc, WithSampler(test.sampler))
} else if test.localParent {
Expand Down Expand Up @@ -189,6 +190,7 @@ func TestStartSpanWithRemoteParent(t *testing.T) {
TraceID: tid,
SpanID: sid,
TraceOptions: 0x0,
Tracestate: &Tracestate{},
}
ctx, _ := StartSpanWithRemoteParent(context.Background(), "startSpanWithRemoteParent", sc)
if err := checkChild(sc, FromContext(ctx)); err != nil {
Expand All @@ -204,6 +206,7 @@ func TestStartSpanWithRemoteParent(t *testing.T) {
TraceID: tid,
SpanID: sid,
TraceOptions: 0x1,
Tracestate: &Tracestate{},
}
ctx, _ = StartSpanWithRemoteParent(context.Background(), "startSpanWithRemoteParent", sc)
if err := checkChild(sc, FromContext(ctx)); err != nil {
Expand All @@ -229,6 +232,7 @@ func startSpan(o StartOptions) *Span {
TraceID: tid,
SpanID: sid,
TraceOptions: 1,
Tracestate: &Tracestate{},
},
WithSampler(o.Sampler),
WithSpanKind(o.SpanKind),
Expand Down Expand Up @@ -299,6 +303,7 @@ func TestSpanKind(t *testing.T) {
TraceID: tid,
SpanID: SpanID{},
TraceOptions: 0x1,
Tracestate: &Tracestate{},
},
ParentSpanID: sid,
Name: "span0",
Expand All @@ -316,6 +321,7 @@ func TestSpanKind(t *testing.T) {
TraceID: tid,
SpanID: SpanID{},
TraceOptions: 0x1,
Tracestate: &Tracestate{},
},
ParentSpanID: sid,
Name: "span0",
Expand All @@ -333,6 +339,7 @@ func TestSpanKind(t *testing.T) {
TraceID: tid,
SpanID: SpanID{},
TraceOptions: 0x1,
Tracestate: &Tracestate{},
},
ParentSpanID: sid,
Name: "span0",
Expand Down Expand Up @@ -367,6 +374,7 @@ func TestSetSpanAttributes(t *testing.T) {
TraceID: tid,
SpanID: SpanID{},
TraceOptions: 0x1,
Tracestate: &Tracestate{},
},
ParentSpanID: sid,
Name: "span0",
Expand Down Expand Up @@ -398,6 +406,7 @@ func TestAnnotations(t *testing.T) {
TraceID: tid,
SpanID: SpanID{},
TraceOptions: 0x1,
Tracestate: &Tracestate{},
},
ParentSpanID: sid,
Name: "span0",
Expand Down Expand Up @@ -432,6 +441,7 @@ func TestMessageEvents(t *testing.T) {
TraceID: tid,
SpanID: SpanID{},
TraceOptions: 0x1,
Tracestate: &Tracestate{},
},
ParentSpanID: sid,
Name: "span0",
Expand Down Expand Up @@ -510,6 +520,7 @@ func TestSetSpanStatus(t *testing.T) {
TraceID: tid,
SpanID: SpanID{},
TraceOptions: 0x1,
Tracestate: &Tracestate{},
Copy link
Contributor

@semistrict semistrict Aug 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
ParentSpanID: sid,
Name: "span0",
Expand Down Expand Up @@ -539,6 +550,7 @@ func TestAddLink(t *testing.T) {
TraceID: tid,
SpanID: SpanID{},
TraceOptions: 0x1,
Tracestate: &Tracestate{},
},
ParentSpanID: sid,
Name: "span0",
Expand Down
106 changes: 106 additions & 0 deletions trace/tracestate.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
// Copyright 2018, OpenCensus Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package trace

import (
"fmt"
"regexp"
)

const (
keyWithoutVendorFormat = `[a-z][_0-9a-z\-\*\/]{0,255}`
keyWithVendorFormat = `[a-z][_0-9a-z\-\*\/]{0,240}@[a-z][_0-9a-z\-\*\/]{0,13}`
keyFormat = `(` + keyWithoutVendorFormat + `)|(` + keyWithVendorFormat + `)`
valueFormat = `[\x20-\x2b\x2d-\x3c\x3e-\x7e]{0,255}[\x21-\x2b\x2d-\x3c\x3e-\x7e]`
)

var keyValidationRegExp = regexp.MustCompile(`^(` + keyFormat + `)$`)
var valueValidationRegExp = regexp.MustCompile(`^(` + valueFormat + `)$`)

type TracestateEntry struct {
key string
value string
}

func (ts *TracestateEntry) IsValid() bool {
return keyValidationRegExp.MatchString(ts.key) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

valueValidationRegExp.MatchString(ts.value)
}

// 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 {
entries []TracestateEntry
}

func (ts *Tracestate) IsValid() bool {
if len(ts.entries) == 0 || len(ts.entries) > 32 {
return false
}
for _, entry := range ts.entries {
if !entry.IsValid() {
return false
}
}
return true
}

func (ts *Tracestate) Fork() *Tracestate {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

retval := Tracestate{entries: ts.entries}
return &retval
}

func (ts *Tracestate) Get(key string) string {
for _, entry := range ts.entries {
Copy link
Contributor

@semistrict semistrict Aug 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if ts == nil {
return ""
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if entry.key == key {
return entry.value
}
}
return ""
}

/*
TODO: we're making Tracestate immutable for now, ramonza and reyang to figure
out how to change it.
https://github.com/census-instrumentation/opencensus-go/pull/887

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

func (ts *Tracestate) Set(key string, value string) string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll remove it.

retval := ""
newEntry := TracestateEntry{key: key, value: value}
if !newEntry.IsValid() && value != "" {
return retval
}
for index, entry := range ts.entries {
if entry.key == key {
ts.entries = append(ts.entries[:index], ts.entries[index+1:]...)
retval = entry.value
break
}
}
if value != "" {
ts.entries = append([]TracestateEntry{newEntry}, ts.entries...)
}
return retval
}
*/

func (ts *Tracestate) String() string {
return fmt.Sprintf("tracestate%s", ts.entries)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@semistrict semistrict Aug 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}