From 670b0365d80ea59d349bec252b67b15ec2b833a3 Mon Sep 17 00:00:00 2001 From: Krzesimir Nowak Date: Fri, 23 Aug 2019 18:01:52 +0200 Subject: [PATCH] Make tag.Map a concrete type. (#89) This is to make tag.Map an immutable type, so it is safe to use concurrently. The safety is not yet fully achieved because of the functions returning contents of the map (Value and Foreach). The functions give callers an access to core.Value objects, which contain a byte slice, which has pointer like semantics. So to avoid accidental changes, we will need to copy the value if it is of BYTES type. Fixes #59 --- api/tag/{api.go => context.go} | 66 ++------- api/tag/map.go | 127 ++++++++---------- api/tag/mutator.go | 69 ++++++++++ .../exporter/reader/format/format.go | 6 +- .../streaming/exporter/reader/reader.go | 2 +- sdk/trace/tracer.go | 3 +- 6 files changed, 143 insertions(+), 130 deletions(-) rename api/tag/{api.go => context.go} (58%) create mode 100644 api/tag/mutator.go diff --git a/api/tag/api.go b/api/tag/context.go similarity index 58% rename from api/tag/api.go rename to api/tag/context.go index c7250e187b9..b178b6fdb01 100644 --- a/api/tag/api.go +++ b/api/tag/context.go @@ -16,8 +16,7 @@ package tag import ( "context" - - "go.opentelemetry.io/api/core" + "runtime/pprof" ) type ctxTagsType struct{} @@ -26,56 +25,6 @@ var ( ctxTagsKey = &ctxTagsType{} ) -type MutatorOp int - -const ( - INSERT MutatorOp = iota - UPDATE - UPSERT - DELETE -) - -type Mutator struct { - MutatorOp - core.KeyValue - MeasureMetadata -} - -type MeasureMetadata struct { - TTL int // -1 == infinite, 0 == do not propagate -} - -func (m Mutator) WithTTL(hops int) Mutator { - m.TTL = hops - return m -} - -type MapUpdate struct { - SingleKV core.KeyValue - MultiKV []core.KeyValue - SingleMutator Mutator - MultiMutator []Mutator -} - -type Map interface { - Apply(MapUpdate) Map - - Value(core.Key) (core.Value, bool) - HasValue(core.Key) bool - - Len() int - - Foreach(func(kv core.KeyValue) bool) -} - -func NewEmptyMap() Map { - return tagMap{} -} - -func NewMap(update MapUpdate) Map { - return NewEmptyMap().Apply(update) -} - func WithMap(ctx context.Context, m Map) context.Context { return context.WithValue(ctx, ctxTagsKey, m) } @@ -90,5 +39,16 @@ func FromContext(ctx context.Context) Map { if m, ok := ctx.Value(ctxTagsKey).(Map); ok { return m } - return tagMap{} + return NewEmptyMap() +} + +// Note: the golang pprof.Do API forces this memory allocation, we +// should file an issue about that. (There's a TODO in the source.) +func Do(ctx context.Context, f func(ctx context.Context)) { + m := FromContext(ctx) + keyvals := make([]string, 0, 2*len(m.m)) + for k, v := range m.m { + keyvals = append(keyvals, k.Variable.Name, v.value.Emit()) + } + pprof.Do(ctx, pprof.Labels(keyvals...), f) } diff --git a/api/tag/map.go b/api/tag/map.go index 8035470126f..9982627c1b9 100644 --- a/api/tag/map.go +++ b/api/tag/map.go @@ -15,64 +15,91 @@ package tag import ( - "context" - "runtime/pprof" - "go.opentelemetry.io/api/core" ) +type MeasureMetadata struct { + TTL int // -1 == infinite, 0 == do not propagate +} + type tagContent struct { value core.Value meta MeasureMetadata } -type tagMap map[core.Key]tagContent +type rawMap map[core.Key]tagContent + +type Map struct { + m rawMap +} -var _ Map = tagMap{} +type MapUpdate struct { + SingleKV core.KeyValue + MultiKV []core.KeyValue + SingleMutator Mutator + MultiMutator []Mutator +} -func (t tagMap) Apply(update MapUpdate) Map { - m := make(tagMap, len(t)+len(update.MultiKV)+len(update.MultiMutator)) - for k, v := range t { - m[k] = v +func newMap(raw rawMap) Map { + return Map{ + m: raw, + } +} + +func NewEmptyMap() Map { + return newMap(nil) +} + +func NewMap(update MapUpdate) Map { + return NewEmptyMap().Apply(update) +} + +func (m Map) Apply(update MapUpdate) Map { + r := make(rawMap, len(m.m)+len(update.MultiKV)+len(update.MultiMutator)) + for k, v := range m.m { + r[k] = v } if update.SingleKV.Key.Defined() { - m[update.SingleKV.Key] = tagContent{ + r[update.SingleKV.Key] = tagContent{ value: update.SingleKV.Value, } } for _, kv := range update.MultiKV { - m[kv.Key] = tagContent{ + r[kv.Key] = tagContent{ value: kv.Value, } } if update.SingleMutator.Key.Defined() { - m.apply(update.SingleMutator) + r.apply(update.SingleMutator) } for _, mutator := range update.MultiMutator { - m.apply(mutator) + r.apply(mutator) + } + if len(r) == 0 { + r = nil } - return m + return newMap(r) } -func (m tagMap) Value(k core.Key) (core.Value, bool) { - entry, ok := m[k] +func (m Map) Value(k core.Key) (core.Value, bool) { + entry, ok := m.m[k] if !ok { entry.value.Type = core.INVALID } return entry.value, ok } -func (m tagMap) HasValue(k core.Key) bool { +func (m Map) HasValue(k core.Key) bool { _, has := m.Value(k) return has } -func (m tagMap) Len() int { - return len(m) +func (m Map) Len() int { + return len(m.m) } -func (m tagMap) Foreach(f func(kv core.KeyValue) bool) { - for k, v := range m { +func (m Map) Foreach(f func(kv core.KeyValue) bool) { + for k, v := range m.m { if !f(core.KeyValue{ Key: k, Value: v.value, @@ -82,10 +109,7 @@ func (m tagMap) Foreach(f func(kv core.KeyValue) bool) { } } -func (m tagMap) apply(mutator Mutator) { - if m == nil { - return - } +func (r rawMap) apply(mutator Mutator) { key := mutator.KeyValue.Key content := tagContent{ value: mutator.KeyValue.Value, @@ -93,57 +117,16 @@ func (m tagMap) apply(mutator Mutator) { } switch mutator.MutatorOp { case INSERT: - if _, ok := m[key]; !ok { - m[key] = content + if _, ok := r[key]; !ok { + r[key] = content } case UPDATE: - if _, ok := m[key]; ok { - m[key] = content + if _, ok := r[key]; ok { + r[key] = content } case UPSERT: - m[key] = content + r[key] = content case DELETE: - delete(m, key) - } -} - -func Insert(kv core.KeyValue) Mutator { - return Mutator{ - MutatorOp: INSERT, - KeyValue: kv, - } -} - -func Update(kv core.KeyValue) Mutator { - return Mutator{ - MutatorOp: UPDATE, - KeyValue: kv, - } -} - -func Upsert(kv core.KeyValue) Mutator { - return Mutator{ - MutatorOp: UPSERT, - KeyValue: kv, - } -} - -func Delete(k core.Key) Mutator { - return Mutator{ - MutatorOp: DELETE, - KeyValue: core.KeyValue{ - Key: k, - }, - } -} - -// Note: the golang pprof.Do API forces this memory allocation, we -// should file an issue about that. (There's a TODO in the source.) -func Do(ctx context.Context, f func(ctx context.Context)) { - m := FromContext(ctx).(tagMap) - keyvals := make([]string, 0, 2*len(m)) - for k, v := range m { - keyvals = append(keyvals, k.Variable.Name, v.value.Emit()) + delete(r, key) } - pprof.Do(ctx, pprof.Labels(keyvals...), f) } diff --git a/api/tag/mutator.go b/api/tag/mutator.go new file mode 100644 index 00000000000..13822108803 --- /dev/null +++ b/api/tag/mutator.go @@ -0,0 +1,69 @@ +// Copyright 2019, OpenTelemetry 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 tag + +import ( + "go.opentelemetry.io/api/core" +) + +type MutatorOp int + +const ( + INSERT MutatorOp = iota + UPDATE + UPSERT + DELETE +) + +type Mutator struct { + MutatorOp + core.KeyValue + MeasureMetadata +} + +func (m Mutator) WithTTL(hops int) Mutator { + m.TTL = hops + return m +} + +func Insert(kv core.KeyValue) Mutator { + return Mutator{ + MutatorOp: INSERT, + KeyValue: kv, + } +} + +func Update(kv core.KeyValue) Mutator { + return Mutator{ + MutatorOp: UPDATE, + KeyValue: kv, + } +} + +func Upsert(kv core.KeyValue) Mutator { + return Mutator{ + MutatorOp: UPSERT, + KeyValue: kv, + } +} + +func Delete(k core.Key) Mutator { + return Mutator{ + MutatorOp: DELETE, + KeyValue: core.KeyValue{ + Key: k, + }, + } +} diff --git a/experimental/streaming/exporter/reader/format/format.go b/experimental/streaming/exporter/reader/format/format.go index 45a300aa910..c73ebf5cd0e 100644 --- a/experimental/streaming/exporter/reader/format/format.go +++ b/experimental/streaming/exporter/reader/format/format.go @@ -55,7 +55,7 @@ func AppendEvent(buf *strings.Builder, data reader.Event) { } else { buf.WriteString(" <") f(false)(parentSpanIDKey.String(data.Parent.SpanIDString())) - if data.ParentAttributes != nil { + if data.ParentAttributes.Len() > 0 { data.ParentAttributes.Foreach(f(false)) } buf.WriteString(" >") @@ -113,10 +113,10 @@ func AppendEvent(buf *strings.Builder, data reader.Event) { // Attach the scope (span) attributes and context tags. buf.WriteString(" [") - if data.Attributes != nil { + if data.Attributes.Len() > 0 { data.Attributes.Foreach(f(false)) } - if data.Tags != nil { + if data.Tags.Len() > 0 { data.Tags.Foreach(f(true)) } if data.SpanContext.HasSpanID() { diff --git a/experimental/streaming/exporter/reader/reader.go b/experimental/streaming/exporter/reader/reader.go index 95204fe998c..c353e9ecaa6 100644 --- a/experimental/streaming/exporter/reader/reader.go +++ b/experimental/streaming/exporter/reader/reader.go @@ -311,7 +311,7 @@ func (ro *readerObserver) addMeasurement(e *Event, m stats.Measurement) { func (ro *readerObserver) readMeasureScope(m stats.Measure) (tag.Map, *readerSpan) { // TODO - return nil, nil + return tag.NewEmptyMap(), nil } func (ro *readerObserver) readScope(id observer.ScopeID) (tag.Map, *readerSpan) { diff --git a/sdk/trace/tracer.go b/sdk/trace/tracer.go index 33aa0012967..a4afd715a48 100644 --- a/sdk/trace/tracer.go +++ b/sdk/trace/tracer.go @@ -18,6 +18,7 @@ import ( "context" "go.opentelemetry.io/api/core" + "go.opentelemetry.io/api/tag" apitrace "go.opentelemetry.io/api/trace" ) @@ -94,5 +95,5 @@ func (tr *tracer) WithComponent(component string) apitrace.Tracer { } func (tr *tracer) Inject(ctx context.Context, span apitrace.Span, injector apitrace.Injector) { - injector.Inject(span.SpanContext(), nil) + injector.Inject(span.SpanContext(), tag.NewEmptyMap()) }