refactor(ddtrace/tracer): internalize {Get,Set}GlobalTracer API by darccio · Pull Request #3482 · DataDog/dd-trace-go

darccio · 2025-05-02T15:14:22Z

What does this PR do?

Brings back {Get,Set}GlobalTracer by using generics and leverage the compiler so the API remains functionally identical and stores tracer.Tracer values without creating an import cycle between ddtrace/tracer and ddtrace/internal.

In v1.x we don't have this issue, neither the API exported, because both packages use interfaces defined in ddtrace. As part of our v2 work, we removed some interfaces and migrated others in #2408.

Motivation

Reduce API surface to the essentially required.

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.
New code is free of linting errors. You can check this by running golangci-lint run locally.
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.

Unsure? Have a question? Request a review!

darccio · 2025-05-02T15:16:36Z


 // Stop deactivates the mock tracer and sets the active tracer to a no-op.
 func (t *mocktracer) Stop() {
-	tracer.StopTestTracer()


tracer.StopTestTracer functionality is covered by tracer.Stop.

This change uncovered some tests that were doing weird stuff with the mocktracer, like stacking multiple instances through mocktracer.Start and defer mt.Stop.

darccio · 2025-05-02T15:17:31Z

-	tracer.SetGlobalTracer(t)
-	return t
+	var t tracer.Tracer = newMockTracer()
+	internal.SetGlobalTracer(t)


internal.SetGlobalTracer swaps and stops the previously set tracer, so it is functionally equal.

darccio · 2025-05-02T15:21:10Z

+}
+
+func getGlobalTracer() tracer.Tracer {
+	return internal.GetGlobalTracer[tracer.Tracer]()


API users may find this a bit uncomfortable but it's better to have a bit of duplication than perfect ergonomics but leaky abstractions.

datadog-datadog-prod-us1 · 2025-05-02T15:22:26Z

Datadog Report

Branch report: dario.castane/internalize-globaltracer-api
Commit report: 7446f6a
Test service: dd-trace-go

✅ 0 Failed, 4525 Passed, 65 Skipped, 3m 41.05s Total Time

pr-commenter · 2025-05-02T16:14:21Z

Benchmarks

Benchmark execution time: 2025-05-07 10:54:21

Comparing candidate commit 02063e7 in PR branch dario.castane/internalize-globaltracer-api with baseline commit 332f786 in branch main.

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

scenario:BenchmarkSetTagMetric-24

🟥 execution_time [+2.621ns; +4.419ns] or [+2.233%; +3.766%]

…e globaltracer with generics

…ce to globaltracer.go

…ings to reset instead of stop

…csSettings to reset instead of stop

…o reset instead of stop

kakkoyun · 2025-05-05T09:10:18Z

+
+// StoreGlobalTracer stores a tracer in the global tracer.
+// It is the responsibility of the caller to ensure that the value is `tracer.Tracer`.
+func StoreGlobalTracer[T tracerLike](t T) {


Do we actually need an additional StoreGlobalTracer method? Can't we just use the SetGlobalTracer? What's the additional benefit?

This replaces the original Store operation at the init function. I agree it could be replaced by the SetGlobalTracer, but I'm not sure why it was decided to go this way.

kakkoyun · 2025-05-05T09:10:48Z

+// It is the responsibility of the caller to ensure that the value is `tracer.Tracer`.
+func SetGlobalTracer[T tracerLike](t T) {
+	old := *globalTracer.Swap(&t).(*T)
+	old.Stop()


We should have a nil-check here, in case the old value was nil.

Done in 9f8e598.

…es; drop internal.StoreGlobalTracer

hannahkm · 2025-05-05T15:22:21Z

+// It is the responsability of the caller to ensure that calling code uses `tracer.Tracer`
+// as generic type.
+func GetGlobalTracer[T tracerLike]() T {
+	return *globalTracer.Load().(*T)


Should we also be checking for a nil Load() return here?

In the context of our tracer, this shouldn't happen. The init function ensures globalTracer is set. If somebody misuses the API and sets a nil value, which is possible, the check would make sense.

The problem with the check in this PR is that... What should we return? Returning nil would lead to panics when the tracer API is used. Another way would be to set a NoopTracer, but that would create an import cycle between internal and tracer.

I think the best way would be to panic if SetGlobalTracer is called with a nil value. A nil tracer could be considered a bad state that is not recoverable. WDYT? cc @kakkoyun

@darccio I have no strong opinion about panicking in the SetGlobalTracer when a nil value is passed but we could also do nothing when called with a nil instead of panicking.

@genesor My fear by doing nothing is that it might go unnoticed. Another way would be to log that a nil tracer has been passed and we avoided setting it. WDYT?

@darccio I'd go for the log, that's less "radical" than a panic IMO

@genesor Now I realized that this being an internal package, it doesn't have access to our logger. If we use log as provided by the stdlib, it would be a deviation from the rest of the tracer.

I'm reconsidering going the panic route.

IMO an early panic is acceptable in this path. If user miss uses the API, we can fail fast.

But using the NoopTracer and having a warning would be ideal. How was the previous behaviour?

Panic route taken in 02063e7

The explanation: in v1 it was also possible to set a nil tracer. It causes the code to panic once a span is started. I feel it's better to fail on initialization than during the execution.

…il tracer

darccio · 2025-05-07T13:00:22Z

Failing smoke tests fixed here: #3504

darccio requested a review from a team as a code owner May 2, 2025 15:14

darccio commented May 2, 2025

View reviewed changes

darccio requested a review from a team as a code owner May 2, 2025 15:44

darccio added 8 commits May 2, 2025 19:07

refactor(ddtrace/tracer): remove StopTestTracer

d507cab

refactor(ddtrace/tracer): introduce GlobalTracer in internal package

6d2a443

refactor(ddtrace/internal): implement a decoupled API for managing th…

490f12f

…e globaltracer with generics

refactor(ddtrace/tracer): unexport {Get,Set}GlobalTracer

0a20dd1

chore(ddtrace/internal): add licensing information and copyright noti…

3626106

…ce to globaltracer.go

fix(test/gintrace): update mock tracer usage in TestResourceNamerSett…

8f93776

…ings to reset instead of stop

fix(contrib/gorm.io/gorm.v1): update mock tracer usage in TestAnalyti…

9f54096

…csSettings to reset instead of stop

fix(contrib/gorm.io/gorm.v1): update mock tracer usage in TestError t…

5772968

…o reset instead of stop

darccio force-pushed the dario.castane/internalize-globaltracer-api branch from f40bde6 to 5772968 Compare May 2, 2025 17:07

darccio mentioned this pull request May 5, 2025

(feat) internal/civisibility: civisibility mocktracer #3455

Closed

7 tasks

kakkoyun reviewed May 5, 2025

View reviewed changes

refactor(ddtrace/internal): improve SetGlobalTracer to handle nil cas…

9f8e598

…es; drop internal.StoreGlobalTracer

darccio force-pushed the dario.castane/internalize-globaltracer-api branch from 5b79c46 to 9f8e598 Compare May 5, 2025 15:01

fix(ddtrace/internal): correct comment typo in globaltracer.go

e4a722f

hannahkm reviewed May 5, 2025

View reviewed changes

kakkoyun approved these changes May 6, 2025

View reviewed changes

fix(ddtrace/internal): add nil check in SetGlobalTracer to panic on n…

02063e7

…il tracer

hannahkm approved these changes May 7, 2025

View reviewed changes

Merge branch 'main' into dario.castane/internalize-globaltracer-api

7585273

darccio enabled auto-merge (squash) May 7, 2025 12:58

darccio merged commit 281ec7d into main May 7, 2025

darccio deleted the dario.castane/internalize-globaltracer-api branch May 7, 2025 13:11

darccio added a commit that referenced this pull request May 8, 2025

refactor(ddtrace/tracer): internalize {Get,Set}GlobalTracer API (#3482)

9b55799

Uh oh!

Conversation

darccio commented May 2, 2025

What does this PR do?

Motivation

Reviewer's Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darccio May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

datadog-datadog-prod-us1 Bot commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Datadog Report

Uh oh!

pr-commenter Bot commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

scenario:BenchmarkSetTagMetric-24

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darccio May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darccio commented May 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

darccio May 2, 2025 •

edited

Loading

datadog-datadog-prod-us1 Bot commented May 2, 2025 •

edited

Loading

pr-commenter Bot commented May 2, 2025 •

edited

Loading

darccio May 7, 2025 •

edited

Loading