ddtrace/tracer: setup GLS-stored context #2761

eliottness · 2024-06-26T09:17:56Z

What does this PR do?

This PR adds a new internal package called orchestrion to better interop with orchestrion. It's first feature is to interact with a compile-time added field in runtime.g to act as a Goroutine Local Storage (GLS) the same way C does with it's Thread Local Storage (TLS).

Sister PR of DataDog/orchestrion#127

From there the integration of this new package cascasdes into:

the ddtrace/tracer/context.go and internal/trace_context.go files to support storing and reading spans/traces from the GLS
the internal/appsec/dyngo package to refactor and add support for context-stored appsec operations
the internal/appsec/emitter/... and the graphql contribs packages to move the old way of storing context values in the new "dyngo"-way

Motivation

Auto-instrumentation comes and breaks the current way that spans are passed around using Go context.Context key value store. Using the GLS is the appropriate solution to have a better coverage where we can't pass a context around.

Reviewer's Checklist

For every reviewer, this seems interesting to read the internal/orchestrion package first.
For the appsec reviewer (@RomainMuller) reading what's inside internal/appsec/dyngo second also seems useful.

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.

Unsure? Have a question? Request a review!

pr-commenter · 2024-06-26T09:44:37Z

Benchmarks

Benchmark execution time: 2024-07-10 15:08:06

Comparing candidate commit c82c374 in PR branch eliott.bouhana/APPSEC-53654 with baseline commit 6f26c4c in branch main.

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

scenario:BenchmarkOTelApiWithCustomTags/otel_api-24

🟥 execution_time [+223.324ns; +294.876ns] or [+2.979%; +3.934%]

scenario:BenchmarkStartSpanConcurrent-24

🟥 execution_time [+120.618ns; +270.582ns] or [+2.235%; +5.013%]

internal/orchestrion/context_test.go

internal/orchestrion/context.go

internal/orchestrion/gls.go

darccio · 2024-06-28T08:29:59Z

I'll approve it when Julio's suggestions are included and the PR is back to open again 😁

eliottness · 2024-06-28T08:52:32Z

I believe my PR is missing a crucial component so I will put it back to draft

RomainMuller

Mostly stylistic comments, and some ergonomics -- some of which we have discussed offline but I left there "for posterity", but you can feel free to ignore where we essentially agree they are difficult to implement without causing a QoL degradation somewhere else.

internal/appsec/dyngo/operation.go

internal/appsec/emitter/graphqlsec/execution.go

internal/appsec/emitter/graphqlsec/request.go

internal/orchestrion/context.go

internal/orchestrion/context_stack.go

darccio

LGTM in general. Let's just change orchestrion.FromCtxOrGLS to orchestrion.WrapContext.

Signed-off-by: Eliott Bouhana <[email protected]>

… as glsContext Signed-off-by: Eliott Bouhana <[email protected]>

Signed-off-by: Eliott Bouhana <[email protected]>

felixge

Approving this to unblock progress on orchestrion. This is based on:

Monday's call with Romain and Elliot where we discussed the two PRs.
Further review of mine today to ensure that this PR does not pose a risk to existing users of dd-trace-go that don't use orchestrion yet (I took some notes).

One important thing we need going forward is a suite of integration tests that verify that all of this is working as expected, even for the known edge cases.

In particular I'm thinking about scenarios like this:

Parent span finishes before child span(s)
User spawns a goroutine that calls an orchestrion instrumentation method without passing an explicit ctx.
etc.

⚠️ The tests are failing as I'm writing this. Seems like a data race. This should be fixed before merging.

felixge · 2024-07-10T10:24:58Z

contrib/99designs/gqlgen/tracer.go

@@ -104,12 +104,12 @@ func (t *gqlTracer) Validate(_ graphql.ExecutableSchema) error {
 func (t *gqlTracer) InterceptOperation(ctx context.Context, next graphql.OperationHandler) graphql.ResponseHandler {
 	opCtx := graphql.GetOperationContext(ctx)
 	span, ctx := t.createRootSpan(ctx, opCtx)
-	ctx, req := graphqlsec.StartRequestOperation(ctx, nil /* root */, span, types.RequestOperationArgs{


Why did we pass nil instead of span to this func before, and why are we changing it now?

I removed a parameter to the function StartRequestOperation because we always passed nil as a second argument to it. Romain also noted this here

felixge · 2024-07-10T12:29:25Z

ddtrace/tracer/span.go

@@ -503,6 +504,7 @@ func (s *span) Finish(opts ...ddtrace.FinishOption) {
 	}

 	s.finish(t)
+	orchestrion.GLSPopValue(sharedinternal.ActiveSpanKey)


What happens if a span finishes before its children?

I don't see a case where this could happen in the same goroutine. 🤔 In a multiple goroutine setup where the parent ends in another gouroutine this is a real issue that is not taken care of today. We definitively need a kind of cross goroutine GLS in the future

Signed-off-by: Eliott Bouhana <[email protected]>

In order to be able to lazily link against GLS accessors, these need to be declared as variables instead of functions. This allows creation of bindings on the other side that can be conditionally satisffied by the existence of initialized declarations somewhere else. Adds an integration test to demonstrate it is possible to build programs with and without the runtime modification using this technique. Enables: DataDog/dd-trace-go#2761 --------- Signed-off-by: Eliott Bouhana <[email protected]> Signed-off-by: github-actions on behalf of eliottness <[email protected]> Co-authored-by: Eliott Bouhana <[email protected]> Co-authored-by: github-actions on behalf of eliottness <[email protected]>

eliottness force-pushed the eliott.bouhana/APPSEC-53654 branch from 07c2865 to 6d925c5 Compare June 26, 2024 13:41

eliottness commented Jun 27, 2024

View reviewed changes

internal/orchestrion/context_test.go Show resolved Hide resolved

eliottness marked this pull request as ready for review June 27, 2024 17:12

eliottness requested review from a team as code owners June 27, 2024 17:12

eliottness added the appsec label Jun 27, 2024

eliottness changed the title ~~orchestrion: setup GLS-stored context~~ internal/orchestrion: setup GLS-stored context Jun 27, 2024

Julio-Guerra reviewed Jun 28, 2024

View reviewed changes

internal/orchestrion/context.go Outdated Show resolved Hide resolved

Julio-Guerra reviewed Jun 28, 2024

View reviewed changes

internal/orchestrion/gls.go Outdated Show resolved Hide resolved

Julio-Guerra requested review from felixge and darccio June 28, 2024 07:57

Julio-Guerra changed the title ~~internal/orchestrion: setup GLS-stored context~~ ddtrace/tracer: setup GLS-stored context Jun 28, 2024

Julio-Guerra marked this pull request as draft June 28, 2024 08:01

RomainMuller mentioned this pull request Jun 28, 2024

fix: cannot lazily link against GLS accessors DataDog/orchestrion#127

Merged

eliottness force-pushed the eliott.bouhana/APPSEC-53654 branch from 8a805e3 to 8ca3a8c Compare July 1, 2024 12:18

github-actions bot added the apm:ecosystem contrib/* related feature requests or bugs label Jul 1, 2024

RomainMuller reviewed Jul 2, 2024

View reviewed changes

eliottness marked this pull request as ready for review July 2, 2024 15:04

eliottness requested a review from a team as a code owner July 2, 2024 15:04

eliottness requested a review from RomainMuller July 2, 2024 15:06

darccio approved these changes Jul 3, 2024

View reviewed changes

eliottness force-pushed the eliott.bouhana/APPSEC-53654 branch from c6953e3 to 18f3f92 Compare July 3, 2024 08:34

RomainMuller approved these changes Jul 3, 2024

View reviewed changes

rarguelloF approved these changes Jul 3, 2024

View reviewed changes

eliottness added 2 commits July 10, 2024 11:31

orchestrion: setup GLS-stored context

52b09e6

Signed-off-by: Eliott Bouhana <[email protected]>

refactor tests

14bb855

Signed-off-by: Eliott Bouhana <[email protected]>

eliottness added 5 commits July 10, 2024 11:31

trying to get back the perfs of ddtrace/tracer back

ee9f9e1

Signed-off-by: Eliott Bouhana <[email protected]>

new version of internal/orchestrion: implement our of context.Context…

c6fd1c1

… as glsContext Signed-off-by: Eliott Bouhana <[email protected]>

appsec/dyngo: add context managnment in dyngo

c9f5b4b

Signed-off-by: Eliott Bouhana <[email protected]>

suggestions from @RomainMuller

c674507

Signed-off-by: Eliott Bouhana <[email protected]>

rename FromCtxOrGLS to WrapContext

222de37

Signed-off-by: Eliott Bouhana <[email protected]>

eliottness force-pushed the eliott.bouhana/APPSEC-53654 branch 2 times, most recently from 204abc9 to 1163073 Compare July 10, 2024 11:34

felixge approved these changes Jul 10, 2024

View reviewed changes

still run context.WithValue even with orchestrion enabled

b33d2bf

Signed-off-by: Eliott Bouhana <[email protected]>

eliottness force-pushed the eliott.bouhana/APPSEC-53654 branch from 1163073 to b33d2bf Compare July 10, 2024 13:07

Merge branch 'main' into eliott.bouhana/APPSEC-53654

c82c374

eliottness enabled auto-merge (squash) July 10, 2024 14:45

auto-merge was automatically disabled July 10, 2024 14:49
Pull Request is not mergeable

eliottness merged commit 577c776 into main Jul 10, 2024
185 checks passed

eliottness deleted the eliott.bouhana/APPSEC-53654 branch July 10, 2024 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddtrace/tracer: setup GLS-stored context #2761

ddtrace/tracer: setup GLS-stored context #2761

eliottness commented Jun 26, 2024 •

edited

Loading

pr-commenter bot commented Jun 26, 2024 •

edited

Loading

darccio commented Jun 28, 2024

eliottness commented Jun 28, 2024

RomainMuller left a comment

darccio left a comment

felixge left a comment •

edited

Loading

felixge Jul 10, 2024

eliottness Jul 10, 2024

felixge Jul 10, 2024

eliottness Jul 10, 2024 •

edited

Loading

ddtrace/tracer: setup GLS-stored context #2761

ddtrace/tracer: setup GLS-stored context #2761

Conversation

eliottness commented Jun 26, 2024 • edited Loading

What does this PR do?

Motivation

Reviewer's Checklist

pr-commenter bot commented Jun 26, 2024 • edited Loading

Benchmarks

scenario:BenchmarkOTelApiWithCustomTags/otel_api-24

scenario:BenchmarkStartSpanConcurrent-24

darccio commented Jun 28, 2024

eliottness commented Jun 28, 2024

RomainMuller left a comment

Choose a reason for hiding this comment

darccio left a comment

Choose a reason for hiding this comment

felixge left a comment • edited Loading

Choose a reason for hiding this comment

felixge Jul 10, 2024

Choose a reason for hiding this comment

eliottness Jul 10, 2024

Choose a reason for hiding this comment

felixge Jul 10, 2024

Choose a reason for hiding this comment

eliottness Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

eliottness commented Jun 26, 2024 •

edited

Loading

pr-commenter bot commented Jun 26, 2024 •

edited

Loading

felixge left a comment •

edited

Loading

eliottness Jul 10, 2024 •

edited

Loading