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

tracing: Adds support for Span Links #2502

Merged
merged 19 commits into from Feb 1, 2024
Merged

Conversation

mabdinur
Copy link
Contributor

@mabdinur mabdinur commented Jan 11, 2024

What does this PR do?

Implements Span Links in the opentelemetry datadog API

Builds on: https://github.com/DataDog/dd-trace-go/pull/2396/files#diff-a8dc5a7b943aca594d89dc56beced16d9dbc951c6aa0c0ce2d328432dab92c80.

Motivation

Previously setting a span link on an OpenTelemetry span would result in a no-op with this change SpanLinks will be serialized and sent to the Datadog Agent.

Testing

Run the following snippet with a version of the datadog agent that support decoding v0.4 span links:

	tracer.Start(tracer.WithAgentAddr("localhost:8136"))
	defer tracer.Stop()

	// Start a root span
	links := []ddtrace.SpanLink{{TraceID: 1, SpanID: 2}, {TraceID: 3, SpanID: 4, Attributes: map[string]string{"link.name": "alpha_transaction"}, Tracestate: "dd_origin=ci", Flags: 0x80000000}}
	span := tracer.StartSpan("span_with_link", tracer.WithSpanLinks(links))
	span.Finish()
	tracer.Flush()

This generates the following trace level logs in the agent:

2024-02-01 14:24:36 EST | TRACE | TRACE | (pkg/trace/writer/trace.go:209 in addSpans) | Writer: handling new tracer payload with 1 spans: languageName:"go" languageVersion:"1.21.6" tracerVersion:"v1.61.0-dev" chunks:{priority:1 spans:{service:"ddtrace.test" name:"span_with_link" resource:"span_with_link" traceID:6389928532662419093 spanID:6389928532662419093 start:1706815476827049000 duration:34000 meta:{key:"_dd.install.id" value:"35a8c1f1-946f-4a04-b67d-677fb2fd5bd4"} meta:{key:"_dd.install.time" value:"1705966150"} meta:{key:"_dd.install.type" value:"manual"} meta:{key:"_dd.p.dm" value:"-1"} meta:{key:"_dd.p.tid" value:"65bbeff400000000"} meta:{key:"language" value:"go"} meta:{key:"runtime-id" value:"2ee0c4f1-532d-45f5-b7ad-342011775d6c"} metrics:{key:"_dd.agent_psr" value:1} metrics:{key:"_dd.profiling.enabled" value:0} metrics:{key:"_dd.top_level" value:1} metrics:{key:"_dd.trace_span_attribute_schema" value:0} metrics:{key:"_sampling_priority_v1" value:1} metrics:{key:"_top_level" value:1} metrics:{key:"process_id" value:76678} spanLinks:{traceID:1 spanID:2} spanLinks:{traceID:3 spanID:4 attributes:{key:"link.name" value:"alpha_transaction"} tracestate:"dd_origin=ci" flags:2147483648}}}

These links are currently shown in the Datadog UI as "hidden span attributes":

Screenshot 2024-02-01 at 2 35 02 PM

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.
  • Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

For Datadog employees:

  • If this PR touches code that handles credentials of any kind, such as Datadog API keys, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

@mabdinur mabdinur changed the title opentelemetry: Adds support for Span Links [WIP] opentelemetry: adds support for Span Links [WIP] Jan 11, 2024
ddtrace/ddtrace.go Outdated Show resolved Hide resolved
ddtrace/opentelemetry/tracer.go Outdated Show resolved Hide resolved
ddtrace/tracer/span.go Outdated Show resolved Hide resolved
ddtrace/tracer/span.go Outdated Show resolved Hide resolved
@mabdinur mabdinur force-pushed the munir/implement-span-links-poc branch 2 times, most recently from f37b8cc to 3707d3b Compare January 11, 2024 19:12
ddtrace/mocktracer/mockspan.go Outdated Show resolved Hide resolved
ddtrace/mocktracer/mockspan.go Outdated Show resolved Hide resolved
ddtrace/mocktracer/mockspan.go Outdated Show resolved Hide resolved
ddtrace/mocktracer/mockspan.go Outdated Show resolved Hide resolved
ddtrace/mocktracer/mockspan.go Outdated Show resolved Hide resolved
ddtrace/mocktracer/mockspan.go Outdated Show resolved Hide resolved
@mabdinur mabdinur force-pushed the munir/implement-span-links-poc branch from 3707d3b to 01edc9a Compare January 11, 2024 19:56
@pr-commenter
Copy link

pr-commenter bot commented Jan 11, 2024

Benchmarks

Benchmark execution time: 2024-02-01 19:10:30

Comparing candidate commit c75a095 in PR branch munir/implement-span-links-poc with baseline commit e707cc9 in branch main.

Found 0 performance improvements and 12 performance regressions! Performance is the same for 27 metrics, 2 unstable metrics.

scenario:BenchmarkOTelApiWithCustomTags/datadog_otel_api-24

  • 🟥 allocated_mem [+76 bytes; +84 bytes] or [+2.101%; +2.326%]

scenario:BenchmarkPartialFlushing/Disabled-24

  • 🟥 allocated_mem [+9.143MB; +9.430MB] or [+3.014%; +3.108%]

scenario:BenchmarkPartialFlushing/Enabled-24

  • 🟥 allocated_mem [+9.313MB; +13.147MB] or [+2.411%; +3.404%]

scenario:BenchmarkSingleSpanRetention/no-rules-24

  • 🟥 allocated_mem [+4.848KB; +4.849KB] or [+3.087%; +3.087%]
  • 🟥 execution_time [+9.682µs; +11.064µs] or [+3.901%; +4.457%]

scenario:BenchmarkSingleSpanRetention/with-rules/match-all-24

  • 🟥 allocated_mem [+4.732KB; +4.906KB] or [+2.968%; +3.077%]
  • 🟥 execution_time [+9.372µs; +10.696µs] or [+3.745%; +4.274%]

scenario:BenchmarkSingleSpanRetention/with-rules/match-half-24

  • 🟥 allocated_mem [+4.700KB; +4.895KB] or [+2.948%; +3.071%]
  • 🟥 execution_time [+8.437µs; +10.194µs] or [+3.364%; +4.064%]

scenario:BenchmarkStartSpan-24

  • 🟥 allocated_mem [+48 bytes; +48 bytes] or [+3.076%; +3.128%]

scenario:BenchmarkTracerAddSpans-24

  • 🟥 allocated_mem [+49 bytes; +49 bytes] or [+2.212%; +2.212%]
  • 🟥 execution_time [+85.364ns; +131.036ns] or [+2.110%; +3.239%]

ddtrace/tracer/span_msgp.go Outdated Show resolved Hide resolved
ddtrace/tracer/span_msgp.go Outdated Show resolved Hide resolved
ddtrace/tracer/span_msgp.go Outdated Show resolved Hide resolved
ddtrace/tracer/span_msgp.go Outdated Show resolved Hide resolved
ddtrace/tracer/span_msgp.go Outdated Show resolved Hide resolved
ddtrace/tracer/span_msgp.go Outdated Show resolved Hide resolved
ddtrace/tracer/span_msgp.go Outdated Show resolved Hide resolved
ddtrace/tracer/span_msgp.go Outdated Show resolved Hide resolved
ddtrace/tracer/span_msgp.go Outdated Show resolved Hide resolved
ddtrace/tracer/span_msgp.go Outdated Show resolved Hide resolved
@mabdinur mabdinur force-pushed the munir/implement-span-links-poc branch from e9a5b3b to b8467aa Compare January 25, 2024 20:15
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

golangci

ddtrace/ddtrace_gen.go|714 col 23| z.Parent.Msgsize undefined (type SpanContext has no field or method Msgsize)
ddtrace/ddtrace_gen.go|721 col 43| z.Context.Msgsize undefined (type context.Context has no field or method Msgsize)
ddtrace/ddtrace_gen.go|721 col 43| too many errors) (typecheck)
ddtrace/ddtrace_gen.go|393 col 19| z.Parent.DecodeMsg undefined (type SpanContext has no field or method DecodeMsg)
ddtrace/ddtrace_gen.go|441 col 20| z.Context.DecodeMsg undefined (type context.Context has no field or method DecodeMsg)
ddtrace/ddtrace_gen.go|484 col 17| z.Parent.EncodeMsg undefined (type SpanContext has no field or method EncodeMsg)
ddtrace/ddtrace_gen.go|536 col 18| z.Context.EncodeMsg undefined (type context.Context has no field or method EncodeMsg)
ddtrace/ddtrace_gen.go|567 col 20| z.Parent.MarshalMsg undefined (type SpanContext has no field or method MarshalMsg)
ddtrace/ddtrace_gen.go|591 col 21| z.Context.MarshalMsg undefined (type context.Context has no field or method MarshalMsg)
ddtrace/ddtrace_gen.go|628 col 24| z.Parent.UnmarshalMsg undefined (type SpanContext has no field or method UnmarshalMsg)
ddtrace/ddtrace_gen.go|676 col 25| z.Context.UnmarshalMsg undefined (type context.Context has no field or method UnmarshalMsg)
ddtrace/ddtrace_gen.go|714 col 23| z.Parent.Msgsize undefined (type SpanContext has no field or method Msgsize)
ddtrace/ddtrace_gen.go|721 col 43| z.Context.Msgsize undefined (type context.Context has no field or method Msgsize)
ddtrace/ddtrace_gen.go|721 col 43| too many errors (typecheck)
ddtrace/ddtrace_gen.go|393 col 19| z.Parent.DecodeMsg undefined (type SpanContext has no field or method DecodeMsg)
ddtrace/ddtrace_gen.go|441 col 20| z.Context.DecodeMsg undefined (type context.Context has no field or method DecodeMsg)
ddtrace/ddtrace_gen.go|484 col 17| z.Parent.EncodeMsg undefined (type SpanContext has no field or method EncodeMsg)
ddtrace/ddtrace_gen.go|536 col 18| z.Context.EncodeMsg undefined (type context.Context has no field or method EncodeMsg)
ddtrace/ddtrace_gen.go|567 col 20| z.Parent.MarshalMsg undefined (type SpanContext has no field or method MarshalMsg)
ddtrace/ddtrace_gen.go|591 col 21| z.Context.MarshalMsg undefined (type context.Context has no field or method MarshalMsg)
ddtrace/ddtrace_gen.go|628 col 24| z.Parent.UnmarshalMsg undefined (type SpanContext has no field or method UnmarshalMsg)
ddtrace/ddtrace_gen.go|676 col 25| z.Context.UnmarshalMsg undefined (type context.Context has no field or method UnmarshalMsg)
ddtrace/ddtrace_gen.go|714 col 23| z.Parent.Msgsize undefined (type SpanContext has no field or method Msgsize)
ddtrace/ddtrace_gen.go|721 col 43| z.Context.Msgsize undefined (type context.Context has no field or method Msgsize)
ddtrace/ddtrace_gen.go|721 col 43| too many errors) (typecheck)

ddtrace/ddtrace_gen.go Outdated Show resolved Hide resolved
ddtrace/ddtrace_gen.go Outdated Show resolved Hide resolved
ddtrace/ddtrace_gen.go Outdated Show resolved Hide resolved
ddtrace/ddtrace_gen.go Outdated Show resolved Hide resolved
ddtrace/ddtrace_gen.go Outdated Show resolved Hide resolved
ddtrace/ddtrace_gen.go Outdated Show resolved Hide resolved
ddtrace/ddtrace_gen.go Outdated Show resolved Hide resolved
ddtrace/ddtrace_gen.go Outdated Show resolved Hide resolved
ddtrace/ddtrace_gen.go Outdated Show resolved Hide resolved
ddtrace/ddtrace_gen.go Outdated Show resolved Hide resolved
@mabdinur mabdinur changed the title opentelemetry: adds support for Span Links [WIP] opentelemetry: adds support for Span Links Jan 26, 2024
@mabdinur mabdinur marked this pull request as ready for review January 26, 2024 14:27
@mabdinur mabdinur requested a review from a team as a code owner January 26, 2024 14:27
ddtrace/mocktracer/mockspan.go Outdated Show resolved Hide resolved
ddtrace/mocktracer/mockspan.go Outdated Show resolved Hide resolved
ddtrace/mocktracer/mockspan.go Outdated Show resolved Hide resolved
ddtrace/ddtrace_gen_test.go Outdated Show resolved Hide resolved
ddtrace/ddtrace_gen_test.go Outdated Show resolved Hide resolved
ddtrace/mocktracer/mockspan.go Outdated Show resolved Hide resolved
ddtrace/mocktracer/mockspan.go Outdated Show resolved Hide resolved
ddtrace/mocktracer/mockspan.go Outdated Show resolved Hide resolved
ddtrace/opentelemetry/tracer.go Outdated Show resolved Hide resolved
ddtrace/opentelemetry/span_test.go Outdated Show resolved Hide resolved
ddtrace/ddtrace_gen_test.go Outdated Show resolved Hide resolved
ddtrace/ddtrace_gen_test.go Outdated Show resolved Hide resolved
ddtrace/ddtrace_gen_test.go Outdated Show resolved Hide resolved
ddtrace/ddtrace_gen_test.go Outdated Show resolved Hide resolved
ddtrace/ddtrace_gen_test.go Outdated Show resolved Hide resolved
ddtrace/ddtrace_gen_test.go Outdated Show resolved Hide resolved
ddtrace/ddtrace_gen_test.go Outdated Show resolved Hide resolved
ddtrace/ddtrace_gen_test.go Outdated Show resolved Hide resolved
ddtrace/ddtrace_gen_test.go Outdated Show resolved Hide resolved
ddtrace/opentelemetry/span_test.go Outdated Show resolved Hide resolved
@mabdinur mabdinur force-pushed the munir/implement-span-links-poc branch from c4c9639 to e86dbac Compare January 26, 2024 14:48
@mabdinur mabdinur requested a review from a team as a code owner January 29, 2024 20:06
@mabdinur mabdinur force-pushed the munir/implement-span-links-poc branch from e1bd0ba to 9116d8d Compare January 29, 2024 20:07
ddtrace/ddtrace.go Outdated Show resolved Hide resolved
ddtrace/ddtrace.go Outdated Show resolved Hide resolved
ddtrace/ddtrace.go Outdated Show resolved Hide resolved
ddtrace/opentelemetry/tracer.go Outdated Show resolved Hide resolved
ddtrace/ddtrace.go Outdated Show resolved Hide resolved
ddtrace/tracer/span.go Outdated Show resolved Hide resolved
ddtrace/tracer/option.go Outdated Show resolved Hide resolved
ddtrace/ddtrace.go Outdated Show resolved Hide resolved
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

golangci

ddtrace/span_link_msgp.go|363 col 19| z.Parent.DecodeMsg undefined (type SpanContext has no field or method DecodeMsg)
ddtrace/span_link_msgp.go|411 col 20| z.Context.DecodeMsg undefined (type context.Context has no field or method DecodeMsg)
ddtrace/span_link_msgp.go|454 col 17| z.Parent.EncodeMsg undefined (type SpanContext has no field or method EncodeMsg)
ddtrace/span_link_msgp.go|506 col 18| z.Context.EncodeMsg undefined (type context.Context has no field or method EncodeMsg)
ddtrace/span_link_msgp.go|533 col 23| z.Parent.Msgsize undefined (type SpanContext has no field or method Msgsize)
ddtrace/span_link_msgp.go|540 col 43| z.Context.Msgsize undefined (type context.Context has no field or method Msgsize)) (typecheck)

ddtrace/span_link_msgp.go Outdated Show resolved Hide resolved
ddtrace/span_link_msgp.go Outdated Show resolved Hide resolved
ddtrace/span_link_msgp.go Outdated Show resolved Hide resolved
ddtrace/span_link_msgp.go Outdated Show resolved Hide resolved
ddtrace/span_link_msgp.go Outdated Show resolved Hide resolved
ddtrace/span_link_msgp.go Outdated Show resolved Hide resolved
ddtrace/span_link_msgp.go Outdated Show resolved Hide resolved
ddtrace/span_link_msgp.go Outdated Show resolved Hide resolved
ddtrace/span_link_msgp.go Outdated Show resolved Hide resolved
ddtrace/span_link_msgp.go Outdated Show resolved Hide resolved
@mabdinur mabdinur force-pushed the munir/implement-span-links-poc branch 2 times, most recently from 94bc4d0 to 0f64408 Compare January 29, 2024 23:16
opentelemetry: Adds support for SpanLinks [WIP]

copy biolerplate for msgpack encoding

serialize otel span links

update mock span and clean up tests

fix broken auto gen tests
revert unnecessary changes
ddtrace/opentelemetry/span_test.go Outdated Show resolved Hide resolved
ddtrace/opentelemetry/span_test.go Outdated Show resolved Hide resolved
ddtrace/opentelemetry/span_test.go Outdated Show resolved Hide resolved
ddtrace/opentelemetry/span_test.go Outdated Show resolved Hide resolved
ddtrace/opentelemetry/span_test.go Outdated Show resolved Hide resolved
ddtrace/opentelemetry/span_test.go Outdated Show resolved Hide resolved
@Julio-Guerra Julio-Guerra removed the request for review from a team January 31, 2024 08:39
@Julio-Guerra
Copy link
Contributor

I removed asm-go from the reviewers as I didn't see anything other than pure APM tracing in this PR.
Let me know if not and need our review.

ddtrace/ddtrace.go Outdated Show resolved Hide resolved
ddtrace/ddtrace.go Outdated Show resolved Hide resolved
ddtrace/ddtrace.go Outdated Show resolved Hide resolved
Copy link
Contributor

@katiehockman katiehockman left a comment

Choose a reason for hiding this comment

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

A few nits for some Go idioms, but otherwise looks good. Thanks!

ddtrace/ddtrace.go Outdated Show resolved Hide resolved
ddtrace/opentelemetry/span_test.go Outdated Show resolved Hide resolved
ddtrace/opentelemetry/tracer.go Outdated Show resolved Hide resolved
ddtrace/opentelemetry/tracer.go Outdated Show resolved Hide resolved
ddtrace/opentelemetry/tracer.go Outdated Show resolved Hide resolved
ddtrace/opentelemetry/tracer.go Outdated Show resolved Hide resolved
ddtrace/tracer/option.go Outdated Show resolved Hide resolved
@mabdinur mabdinur force-pushed the munir/implement-span-links-poc branch from 2c0ac32 to db266fa Compare February 1, 2024 18:46
@mabdinur mabdinur force-pushed the munir/implement-span-links-poc branch from db266fa to c75a095 Compare February 1, 2024 18:56
ddtrace/tracer/tracer.go Outdated Show resolved Hide resolved
ddtrace/tracer/tracer.go Outdated Show resolved Hide resolved
@katiehockman katiehockman merged commit bbbbd7a into main Feb 1, 2024
153 of 154 checks passed
@katiehockman katiehockman deleted the munir/implement-span-links-poc branch February 1, 2024 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants