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

chore(otel): adds an API for generating span links #2614

Merged
merged 7 commits into from Mar 21, 2024

Conversation

mabdinur
Copy link
Contributor

What does this PR do?

Adds AddLink to the span struct.

Motivation

Currently span links can only be set on spans on span creation (via WithSpanLinks option). To support many use cases we need the ability to add SpanLinks after a span has been created.

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 requested a review from a team as a code owner March 13, 2024 15:47
@mabdinur mabdinur marked this pull request as draft March 13, 2024 15:47
@mabdinur mabdinur added this to the v2.0.0 milestone Mar 13, 2024
ddtrace/tracer/span.go Outdated Show resolved Hide resolved
@pr-commenter
Copy link

pr-commenter bot commented Mar 13, 2024

Benchmarks

Benchmark execution time: 2024-03-18 14:59:18

Comparing candidate commit b18de31 in PR branch munir/add-span-link-public-api with baseline commit 0730ca1 in branch v2-dev.

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

scenario:BenchmarkPartialFlushing/Enabled-24

  • 🟥 execution_time [+7.389ms; +11.859ms] or [+2.494%; +4.003%]

@mabdinur mabdinur force-pushed the munir/add-span-link-public-api branch from 853481a to ce049ea Compare March 13, 2024 18:59
@mabdinur mabdinur force-pushed the munir/add-span-link-public-api branch from ce049ea to 3519f3d Compare March 14, 2024 13:54
@mabdinur mabdinur marked this pull request as ready for review March 14, 2024 13:54
@mabdinur
Copy link
Contributor Author

@dianashevchenko Do I also need to update mockspan.go?

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
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
Copy link
Contributor

@dianashevchenko dianashevchenko left a comment

Choose a reason for hiding this comment

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

Nits

ddtrace/tracer/span.go Outdated Show resolved Hide resolved
ddtrace/tracer/span.go Outdated Show resolved Hide resolved
@dianashevchenko
Copy link
Contributor

@mabdinur just remembered this change should probably be reflected in the opentelemetry package as well. Since, OTel API does have the AddLink method, which we couldn't support before.

@mabdinur
Copy link
Contributor Author

mabdinur commented Mar 15, 2024

@mabdinur just remembered this change should probably be reflected in the opentelemetry package as well. Since, OTel API does have the AddLink method, which we couldn't support before.

I am not sure if the Otel API exposes an AddLink method: https://github.com/open-telemetry/opentelemetry-go/blob/main/trace/trace.go#L338. I think adding links to existing spans is a concept internal to the go sdk: https://github.com/open-telemetry/opentelemetry-go/blob/da047e70ef583efbc91883dd2e35c616569260ef/sdk/trace/span.go#L632C25-L632C32.

@dianashevchenko
Copy link
Contributor

@mabdinur yes, I think you're right. Even better then.

@darccio the PR looks good to me, but feel free to review.

@dianashevchenko
Copy link
Contributor

@mabdinur please address my previous comments before marking the conversation as resolved 🙌

Copy link
Contributor

@darccio darccio left a comment

Choose a reason for hiding this comment

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

LGTM

@mabdinur mabdinur merged commit 224a0bc into v2-dev Mar 21, 2024
155 checks passed
@mabdinur mabdinur deleted the munir/add-span-link-public-api branch March 21, 2024 23:00
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