Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Span links improvements and bug fix #3123

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 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
22 changes: 3 additions & 19 deletions ddtrace/ddtrace.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ type Span interface {
// item should propagate to all descendant spans, both in- and cross-process.
SetBaggageItem(key, val string)

// AddSpanLinks appends the given links to the span's span links.
AddSpanLinks(spanLinks ...SpanLink)
Copy link
Contributor

Choose a reason for hiding this comment

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

A few notes:

  1. We already have support for adding span links to a span when you are starting it; do you need this new API because you need to add span links to a span that has already been started? If so, what is the motivation for that? If not, can you use the existing API?
  2. Adding a new method to the existing Span interface is a breaking change. The alternative approach is to introduce a new interface with the new method (with the old interface embedded). You can see what we do for adding spanlinks to the span context here: https://github.com/DataDog/dd-trace-go/pull/2973/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Yes, I do need to add span links to a span that's already started. In my case, I will need to get some values from an S3/Dynamo response before adding span links, which occurs right before the span is finished.
  2. Out of curiosity, why is this a breaking change? Done in 66d3a67, is that what you were looking for?

Copy link
Contributor

@mtoffl01 mtoffl01 Jan 29, 2025

Choose a reason for hiding this comment

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

Public interfaces are considered "contracts" in Golang. Modifying them is "breaking contract" because that means every type which expects to satisfy this interface must now be updated -- this can impact customer code.


// Finish finishes the current span with the given options. Finish calls should be idempotent.
Finish(opts ...FinishOption)

Expand All @@ -103,25 +106,6 @@ type SpanContext interface {
ForeachBaggageItem(handler func(k, v string) bool)
}

// SpanLink represents a reference to a span that exists outside of the trace.
//
//go:generate msgp -unexported -marshal=false -o=span_link_msgp.go -tests=false

type SpanLink struct {
// TraceID represents the low 64 bits of the linked span's trace id. This field is required.
TraceID uint64 `msg:"trace_id" json:"trace_id"`
// TraceIDHigh represents the high 64 bits of the linked span's trace id. This field is only set if the linked span's trace id is 128 bits.
TraceIDHigh uint64 `msg:"trace_id_high,omitempty" json:"trace_id_high"`
// SpanID represents the linked span's span id.
SpanID uint64 `msg:"span_id" json:"span_id"`
// Attributes is a mapping of keys to string values. These values are used to add additional context to the span link.
Attributes map[string]string `msg:"attributes,omitempty" json:"attributes"`
// Tracestate is the tracestate of the linked span. This field is optional.
Tracestate string `msg:"tracestate,omitempty" json:"tracestate"`
// Flags represents the W3C trace flags of the linked span. This field is optional.
Flags uint32 `msg:"flags,omitempty" json:"flags"`
}

// StartSpanOption is a configuration option that can be used with a Tracer's StartSpan method.
type StartSpanOption func(cfg *StartSpanConfig)

Expand Down
3 changes: 3 additions & 0 deletions ddtrace/internal/globaltracer.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,9 @@ func (NoopSpan) BaggageItem(_ string) string { return "" }
// SetBaggageItem implements ddtrace.Span.
func (NoopSpan) SetBaggageItem(_, _ string) {}

// AddSpanLinks implements ddtrace.Span.
func (s NoopSpan) AddSpanLinks(_ ...ddtrace.SpanLink) {}

// Finish implements ddtrace.Span.
func (NoopSpan) Finish(_ ...ddtrace.FinishOption) {}

Expand Down
5 changes: 5 additions & 0 deletions ddtrace/mocktracer/mockspan.go
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,11 @@ func (s *mockspan) SetBaggageItem(key, val string) {
return
}

// AddSpanLinks appends the given links to the span's span links.
func (s *mockspan) AddSpanLinks(spanLinks ...ddtrace.SpanLink) {
nhulston marked this conversation as resolved.
Show resolved Hide resolved
s.links = append(s.links, spanLinks...)
}

// Finish finishes the current span with the given options.
func (s *mockspan) Finish(opts ...ddtrace.FinishOption) {
var cfg ddtrace.FinishConfig
Expand Down
25 changes: 25 additions & 0 deletions ddtrace/span_link.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Unless explicitly stated otherwise all files in this repository are licensed
// under the Apache License Version 2.0.
// This product includes software developed at Datadog (https://www.datadoghq.com/).
// Copyright 2016 Datadog, Inc.

package ddtrace

// SpanLink represents a reference to a span that exists outside of the trace.
//
//go:generate msgp -unexported -marshal=false -o=span_link_msgp.go -tests=false

type SpanLink struct {
// TraceID represents the low 64 bits of the linked span's trace id. This field is required.
TraceID uint64 `msg:"trace_id" json:"trace_id"`
// TraceIDHigh represents the high 64 bits of the linked span's trace id. This field is only set if the linked span's trace id is 128 bits.
TraceIDHigh uint64 `msg:"trace_id_high" json:"trace_id_high"`
// SpanID represents the linked span's span id.
SpanID uint64 `msg:"span_id" json:"span_id"`
// Attributes is a mapping of keys to string values. These values are used to add additional context to the span link.
Attributes map[string]string `msg:"attributes" json:"attributes"`
// Tracestate is the tracestate of the linked span. This field is optional.
Tracestate string `msg:"tracestate" json:"tracestate"`
// Flags represents the W3C trace flags of the linked span. This field is optional.
Flags uint32 `msg:"flags" json:"flags"`
}
120 changes: 43 additions & 77 deletions ddtrace/span_link_msgp.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions ddtrace/tracer/civisibility_tslv.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,15 @@ func (e *ciVisibilityEvent) SetBaggageItem(key, val string) {
e.span.SetBaggageItem(key, val)
}

// AddSpanLinks appends the given links to the span's span links.
//
// Parameters:
//
// spanLinks - list of span links to append.
func (e *ciVisibilityEvent) AddSpanLinks(spanLinks ...ddtrace.SpanLink) {
e.span.SpanLinks = append(e.span.SpanLinks, spanLinks...)
}

// Finish completes the event's span with optional finish options.
//
// Parameters:
Expand Down
23 changes: 23 additions & 0 deletions ddtrace/tracer/span.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ package tracer
import (
"context"
"encoding/base64"
"encoding/json"
"fmt"
"os"
"reflect"
Expand Down Expand Up @@ -464,6 +465,26 @@ func (s *span) setMetric(key string, v float64) {
}
}

// AddSpanLinks appends the given links to the span's span links.
func (s *span) AddSpanLinks(spanLinks ...ddtrace.SpanLink) {
mtoffl01 marked this conversation as resolved.
Show resolved Hide resolved
s.SpanLinks = append(s.SpanLinks, spanLinks...)
}

// serializeSpanLinksInMeta saves span links as a JSON string under `Span[meta][_dd.span_links]`.
func (s *span) serializeSpanLinksInMeta() {
if len(s.SpanLinks) == 0 {
return
mtoffl01 marked this conversation as resolved.
Show resolved Hide resolved
}
spanLinkBytes, err := json.Marshal(s.SpanLinks)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A little concerned about performance, but not sure if there's any other way to do this. We have to serialize the links into a JSON string such as "[{\"trace_id\":0,\"trace_id_high\":0,\"span_id\":0,\"attributes\":{\"link.kind\":\"span-pointer\",\"ptr.dir\":\"d\",\"ptr.hash\":\"eb29cb7d923f904f02bd8b3d85e228ed\",\"ptr.kind\":\"aws.s3.object\"},\"tracestate\":\"\",\"flags\":0}]"

Copy link
Contributor

Choose a reason for hiding this comment

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

You could always add a benchmark for that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually must faster than I expected

cpu: Apple M3 Max
BenchmarkSerializeSpanLinksInMeta
BenchmarkSerializeSpanLinksInMeta-16    	 1253884	       964.4 ns/op

if err != nil {
return
mtoffl01 marked this conversation as resolved.
Show resolved Hide resolved
}
if s.Meta == nil {
s.Meta = make(map[string]string)
}
s.Meta["_dd.span_links"] = string(spanLinkBytes)
}

// Finish closes this Span (but not its children) providing the duration
// of its part of the tracing session.
func (s *span) Finish(opts ...ddtrace.FinishOption) {
Expand Down Expand Up @@ -514,6 +535,8 @@ func (s *span) Finish(opts ...ddtrace.FinishOption) {
}
}

s.serializeSpanLinksInMeta()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you state explicitly why you were not able to rely on json.Marshal and we have to call this custom serializer? You may be correct, I just want to make sure I'm following

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The serializeSpanLinksInMeta func does use json.Marshal; I just extracted the method for easier refactoring in the future. We could remove the serializeSpanLinksInMeta() function and put the logic directly in Finish(), wdyt?


s.finish(t)
orchestrion.GLSPopValue(sharedinternal.ActiveSpanKey)
}
Expand Down
2 changes: 1 addition & 1 deletion ddtrace/tracer/tracer.go
Original file line number Diff line number Diff line change
Expand Up @@ -573,7 +573,7 @@ func (t *tracer) StartSpan(operationName string, options ...ddtrace.StartSpanOpt
noDebugStack: t.config.noDebugStack,
}

span.SpanLinks = append(span.SpanLinks, opts.SpanLinks...)
span.AddSpanLinks(opts.SpanLinks...)

if t.config.hostname != "" {
span.setMeta(keyHostname, t.config.hostname)
Expand Down
Loading