diff --git a/_docs/layouts/shortcodes/menu.html b/_docs/layouts/shortcodes/menu.html index f16216e8..fc29ef06 100644 --- a/_docs/layouts/shortcodes/menu.html +++ b/_docs/layouts/shortcodes/menu.html @@ -9,7 +9,7 @@ {{ .Title }}
- {{ or .Params.subtitle .Summary }} + {{ or .Params.subtitle .Summary | plainify }}
{{ end }} diff --git a/internal/cmd/toolexec.go b/internal/cmd/toolexec.go index f9a34e11..f2703be4 100644 --- a/internal/cmd/toolexec.go +++ b/internal/cmd/toolexec.go @@ -40,7 +40,7 @@ var Toolexec = &cli.Command{ pin.AutoPinOrchestrion() if proxyCmd.ShowVersion() { - log.Tracef("Toolexec version command: %#v\n", proxyCmd) + log.Tracef("Toolexec version command: %#v\n", proxyCmd.Args()) fullVersion, err := toolexec.ComputeVersion(proxyCmd) if err != nil { return err diff --git a/internal/injector/aspect/advice/advice.go b/internal/injector/aspect/advice/advice.go index 4c691003..77798792 100644 --- a/internal/injector/aspect/advice/advice.go +++ b/internal/injector/aspect/advice/advice.go @@ -12,8 +12,6 @@ import ( "github.com/DataDog/orchestrion/internal/injector/aspect/context" ) -const pkgPath = "github.com/DataDog/orchestrion/internal/injector/aspect/advice" - // Advice is the interface abstracting actual AST changes performed by // injections. type Advice interface { diff --git a/internal/injector/aspect/advice/code/template_test.go b/internal/injector/aspect/advice/code/template_test.go index 9b9aa4bd..94c91b4d 100644 --- a/internal/injector/aspect/advice/code/template_test.go +++ b/internal/injector/aspect/advice/code/template_test.go @@ -67,6 +67,10 @@ func (m mockAdviceContext) Package() string { assert.FailNow(m.t, "unexpected method call") return "" } +func (m mockAdviceContext) TestMain() bool { + assert.FailNow(m.t, "unexpected method call") + return false +} func (m mockAdviceContext) Child(dst.Node, string, int) context.AdviceContext { assert.FailNow(m.t, "unexpected method call") return nil diff --git a/internal/injector/aspect/context/context.go b/internal/injector/aspect/context/context.go index 9f2aafbd..5eed7744 100644 --- a/internal/injector/aspect/context/context.go +++ b/internal/injector/aspect/context/context.go @@ -40,6 +40,9 @@ type AspectContext interface { // Package returns the name of the package containing this node. Package() string + // TestMain returns true if the current node is in a synthetic main package. + TestMain() bool + // Release returns this context to the memory pool so that it can be reused // later. Release() @@ -81,6 +84,7 @@ type ( minGoLang *GoLangVersion sourceParser SourceParser importPath string + testMain bool } SourceParser interface { @@ -106,6 +110,8 @@ type ContextArgs struct { // MinGoLang is a pointer to the result value containing the minimum Go // language level required by the compile unit after it has been modified. MinGoLang *GoLangVersion + // TestMain is true when injecting into a synthetic main package. + TestMain bool } // Context returns a new [*context] instance that represents the ndoe at the @@ -122,6 +128,7 @@ func (n *NodeChain) Context(args ContextArgs) *context { minGoLang: args.MinGoLang, sourceParser: args.SourceParser, importPath: args.ImportPath, + testMain: args.TestMain, } return c @@ -155,6 +162,7 @@ func (c *context) Child(node dst.Node, property string, index int) AdviceContext minGoLang: c.minGoLang, sourceParser: c.sourceParser, importPath: c.importPath, + testMain: c.testMain, } return r @@ -208,6 +216,10 @@ func (c *context) Package() string { return c.file.Name.Name } +func (c *context) TestMain() bool { + return c.testMain +} + func (c *context) ParseSource(bytes []byte) (*dst.File, error) { return c.sourceParser.Parse(bytes) } diff --git a/internal/injector/aspect/join/join.go b/internal/injector/aspect/join/join.go index 23168a3c..35c3fa65 100644 --- a/internal/injector/aspect/join/join.go +++ b/internal/injector/aspect/join/join.go @@ -16,8 +16,6 @@ import ( "github.com/dave/dst" ) -const pkgPath = "github.com/DataDog/orchestrion/internal/injector/aspect/join" - // Point is the interface that abstracts selection of nodes where to inject // code. type Point interface { diff --git a/internal/injector/aspect/join/testmain.go b/internal/injector/aspect/join/testmain.go new file mode 100644 index 00000000..48377867 --- /dev/null +++ b/internal/injector/aspect/join/testmain.go @@ -0,0 +1,42 @@ +// 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 2023-present Datadog, Inc. + +package join + +import ( + "github.com/DataDog/orchestrion/internal/fingerprint" + "github.com/DataDog/orchestrion/internal/injector/aspect/context" + "gopkg.in/yaml.v3" +) + +type testMain bool + +// TestMain matches only nodes in ASTs in files that either are (if true), or +// are not (if false) part of a synthetic test main package. +func TestMain(v bool) testMain { + return testMain(v) +} + +func (t testMain) Matches(ctx context.AspectContext) bool { + return ctx.TestMain() == bool(t) +} + +func (testMain) ImpliesImported() []string { + return nil +} + +func (t testMain) Hash(h *fingerprint.Hasher) error { + return h.Named("test-main", fingerprint.Bool(t)) +} + +func init() { + unmarshalers["test-main"] = func(node *yaml.Node) (Point, error) { + var val bool + if err := node.Decode(&val); err != nil { + return nil, err + } + return TestMain(val), nil + } +} diff --git a/internal/injector/builtin/generated_deps.go b/internal/injector/builtin/generated_deps.go index d4718ba3..c03fc14e 100644 --- a/internal/injector/builtin/generated_deps.go +++ b/internal/injector/builtin/generated_deps.go @@ -53,6 +53,7 @@ import ( _ "gopkg.in/DataDog/dd-trace-go.v1/contrib/twitchtv/twirp" _ "gopkg.in/DataDog/dd-trace-go.v1/ddtrace" _ "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" + _ "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/internal" _ "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" _ "gopkg.in/DataDog/dd-trace-go.v1/internal" _ "gopkg.in/DataDog/dd-trace-go.v1/internal/appsec" @@ -72,5 +73,4 @@ import ( _ "os" _ "strconv" _ "strings" - _ "testing" ) diff --git a/internal/injector/builtin/generator/doc.join.test-main.tmpl b/internal/injector/builtin/generator/doc.join.test-main.tmpl new file mode 100644 index 00000000..7c40f085 --- /dev/null +++ b/internal/injector/builtin/generator/doc.join.test-main.tmpl @@ -0,0 +1,10 @@ +
+ Synthetic main package + + {{- if . -}} + Yes + {{- else -}} + No + {{- end -}} + +
diff --git a/internal/injector/builtin/testdata/client/main.go.snap b/internal/injector/builtin/testdata/client/main.go.snap index d0a02a66..5ec31952 100644 --- a/internal/injector/builtin/testdata/client/main.go.snap +++ b/internal/injector/builtin/testdata/client/main.go.snap @@ -18,8 +18,6 @@ import ( __orchestrion_tracer "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" __orchestrion_profiler "gopkg.in/DataDog/dd-trace-go.v1/profiler" __orchestrion_log "log" - __orchestrion_testing "testing" - _ "unsafe" ) //dd:span @@ -27,12 +25,7 @@ import ( func main() { //line { - // Only finalize if is not a test process and if CI Visibility has not been disabled (by the kill switch). - // For a test process the ci visibility instrumentation will finalize the tracer - if !__orchestrion_testing.Testing() || !__dd_civisibility_isCiVisibilityEnabled() { - defer __orchestrion_tracer.Stop() - } - + defer __orchestrion_tracer.Stop() defer __orchestrion_profiler.Stop() } { @@ -73,16 +66,9 @@ func main() { fmt.Println(string(b)) } -//go:linkname __dd_civisibility_isCiVisibilityEnabled gopkg.in/DataDog/dd-trace-go.v1/internal/civisibility/integrations/gotesting.isCiVisibilityEnabled //line -func __dd_civisibility_isCiVisibilityEnabled() bool - func init() { - // Only initialize if is not a test process and if CI Visibility has not been disabled (by the kill switch). - // For a test process the ci visibility instrumentation will initialize the tracer - if !__orchestrion_testing.Testing() || !__dd_civisibility_isCiVisibilityEnabled() { - __orchestrion_tracer.Start(__orchestrion_tracer.WithOrchestrion(map[string]string{"version": ""})) - } + __orchestrion_tracer.Start(__orchestrion_tracer.WithOrchestrion(map[string]string{"version": ""})) } func init() { switch os.Getenv("DD_PROFILING_ENABLED") { diff --git a/internal/injector/builtin/testdata/server/main.go.snap b/internal/injector/builtin/testdata/server/main.go.snap index cb37bad3..1d4b6f4b 100644 --- a/internal/injector/builtin/testdata/server/main.go.snap +++ b/internal/injector/builtin/testdata/server/main.go.snap @@ -14,20 +14,13 @@ import ( __orchestrion_tracer "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" __orchestrion_profiler "gopkg.in/DataDog/dd-trace-go.v1/profiler" __orchestrion_os "os" - __orchestrion_testing "testing" - _ "unsafe" ) //line samples/server/main.go:14 func main() { //line { - // Only finalize if is not a test process and if CI Visibility has not been disabled (by the kill switch). - // For a test process the ci visibility instrumentation will finalize the tracer - if !__orchestrion_testing.Testing() || !__dd_civisibility_isCiVisibilityEnabled() { - defer __orchestrion_tracer.Stop() - } - + defer __orchestrion_tracer.Stop() defer __orchestrion_profiler.Stop() } //line samples/server/main.go:15 @@ -68,16 +61,9 @@ func instrumentedHandler(w http.ResponseWriter, r *http.Request) { // comment that is just hanging out unattached // -//go:linkname __dd_civisibility_isCiVisibilityEnabled gopkg.in/DataDog/dd-trace-go.v1/internal/civisibility/integrations/gotesting.isCiVisibilityEnabled //line -func __dd_civisibility_isCiVisibilityEnabled() bool - func init() { - // Only initialize if is not a test process and if CI Visibility has not been disabled (by the kill switch). - // For a test process the ci visibility instrumentation will initialize the tracer - if !__orchestrion_testing.Testing() || !__dd_civisibility_isCiVisibilityEnabled() { - __orchestrion_tracer.Start(__orchestrion_tracer.WithOrchestrion(map[string]string{"version": ""})) - } + __orchestrion_tracer.Start(__orchestrion_tracer.WithOrchestrion(map[string]string{"version": ""})) } func init() { switch __orchestrion_os.Getenv("DD_PROFILING_ENABLED") { diff --git a/internal/injector/builtin/yaml/go-main.yml b/internal/injector/builtin/yaml/go-main.yml index 9d972726..155f533f 100644 --- a/internal/injector/builtin/yaml/go-main.yml +++ b/internal/injector/builtin/yaml/go-main.yml @@ -13,31 +13,22 @@ aspects: join-point: all-of: - package-name: main + - test-main: false - function-body: function: - name: main - signature: {} advice: - inject-declarations: - links: - - gopkg.in/DataDog/dd-trace-go.v1/internal/civisibility/integrations/gotesting imports: tracer: gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer - testing: testing # Note: it is valid to have multiple func init() in a single compile unit (e.g, `.go` file), in which case # they get executed in declaration order. This means it's okay for us to add a new init function if there is # already one in the file, but as it currently is appended AFTER all other declarations in the file, it means # that it will be executed last (tracing contents of previous init functions will not be possible). template: |- - //go:linkname __dd_civisibility_isCiVisibilityEnabled gopkg.in/DataDog/dd-trace-go.v1/internal/civisibility/integrations/gotesting.isCiVisibilityEnabled - func __dd_civisibility_isCiVisibilityEnabled() bool - func init() { - // Only initialize if is not a test process and if CI Visibility has not been disabled (by the kill switch). - // For a test process the ci visibility instrumentation will initialize the tracer - if !testing.Testing() || !__dd_civisibility_isCiVisibilityEnabled() { - tracer.Start(tracer.WithOrchestrion(map[string]string{"version": {{printf "%q" Version}}})) - } + tracer.Start(tracer.WithOrchestrion(map[string]string{"version": {{printf "%q" Version}}})) } - inject-declarations: @@ -84,12 +75,6 @@ aspects: imports: profiler: gopkg.in/DataDog/dd-trace-go.v1/profiler tracer: gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer - testing: testing template: |- - // Only finalize if is not a test process and if CI Visibility has not been disabled (by the kill switch). - // For a test process the ci visibility instrumentation will finalize the tracer - if !testing.Testing() || !__dd_civisibility_isCiVisibilityEnabled() { - defer tracer.Stop() - } - + defer tracer.Stop() defer profiler.Stop() diff --git a/internal/injector/builtin/yaml/stdlib/runtime.yml b/internal/injector/builtin/yaml/stdlib/runtime.yml index 2f26354b..dc8f3c06 100644 --- a/internal/injector/builtin/yaml/stdlib/runtime.yml +++ b/internal/injector/builtin/yaml/stdlib/runtime.yml @@ -46,3 +46,31 @@ aspects: advice: - prepend-statements: template: getg().__dd_gls = nil + + # Temporarily hack tracer.SpanFromContext so that it no longer returns true if + # the Span found from GLS is actually a noop span. + # This might happen when a library creates spans using the OTel SDK. + - id: Hack tracer.SpanFromContext + tracer-internal: true + join-point: + all-of: + - import-path: gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer + - function-body: + function: + - name: SpanFromContext + advice: + - prepend-statements: + imports: + traceinternal: gopkg.in/DataDog/dd-trace-go.v1/ddtrace/internal + template: |- + {{- $span := .Function.Result 0 -}} + {{- $ok := .Function.Result 1 -}} + defer func(){ + if !{{ $ok }} { + return + } + switch {{ $span }}.(type) { + case traceinternal.NoopSpan, *traceinternal.NoopSpan: + {{ $ok }} = false + } + }() diff --git a/internal/injector/config/config_test.go b/internal/injector/config/config_test.go index 308fc206..2c623455 100644 --- a/internal/injector/config/config_test.go +++ b/internal/injector/config/config_test.go @@ -186,7 +186,7 @@ func TestLoad(t *testing.T) { enc.SetIndent(2) require.NoError(t, enc.Encode(cfg)) - assert.Len(t, cfg.Aspects(), 3) + assert.Len(t, cfg.Aspects(), 4) golden.Assert(t, buf.String(), "required.snap.yml") }) @@ -201,7 +201,7 @@ func TestLoad(t *testing.T) { enc.SetIndent(2) require.NoError(t, enc.Encode(cfg)) - assert.Len(t, cfg.Aspects(), 105) + assert.Len(t, cfg.Aspects(), 106) golden.Assert(t, buf.String(), "instrument.snap.yml") }) @@ -232,7 +232,7 @@ func TestLoad(t *testing.T) { loader := NewLoader(tmp, false) cfg, err := loader.Load() require.NoError(t, err) - require.Len(t, cfg.Aspects(), 4) + require.Len(t, cfg.Aspects(), 5) }) } diff --git a/internal/injector/config/schema.json b/internal/injector/config/schema.json index 16554432..60118698 100644 --- a/internal/injector/config/schema.json +++ b/internal/injector/config/schema.json @@ -377,6 +377,7 @@ { "$ref": "#/$defs/join-point/package-name" }, { "$ref": "#/$defs/join-point/struct-definition" }, { "$ref": "#/$defs/join-point/struct-literal" }, + { "$ref": "#/$defs/join-point/test-main" }, { "$ref": "#/$defs/join-point/value-declaration" } ] }, @@ -686,6 +687,17 @@ } ] }, + "test-main": { + "required": ["test-main"], + "unevaluatedProperties": false, + "properties": { + "test-main": { + "title": "Synthetic test main package", + "markdownDescription": "The `test-main` join point can be used to only (or never) match nodes included in the synthetic main package generated by `go test`.", + "type": "boolean" + } + } + }, "value-declaration": { "required": ["value-declaration"], "unevaluatedProperties": false, diff --git a/internal/injector/config/testdata/instrument.snap.yml b/internal/injector/config/testdata/instrument.snap.yml index 15e1ade3..10050bd5 100644 --- a/internal/injector/config/testdata/instrument.snap.yml +++ b/internal/injector/config/testdata/instrument.snap.yml @@ -11,6 +11,7 @@ imports: aspects: - GLS Access - Clear GLS slot on goroutine exit + - Hack tracer.SpanFromContext yaml: name: orchestrion.yml extends: diff --git a/internal/injector/config/testdata/required.snap.yml b/internal/injector/config/testdata/required.snap.yml index f072101e..0ad73a70 100644 --- a/internal/injector/config/testdata/required.snap.yml +++ b/internal/injector/config/testdata/required.snap.yml @@ -11,4 +11,5 @@ imports: aspects: - GLS Access - Clear GLS slot on goroutine exit + - Hack tracer.SpanFromContext yaml: null diff --git a/internal/injector/injector.go b/internal/injector/injector.go index 6987bf75..8d324ae9 100644 --- a/internal/injector/injector.go +++ b/internal/injector/injector.go @@ -39,6 +39,8 @@ type ( // GoVersion is the go runtime version required by this package. If blank, no go runtime compatibility will be // asserted. GoVersion string + // TestMain must be set to true when injecting into the generated test main package. + TestMain bool // ModifiedFile is called to determine the output file name for a modified file. If nil, the input file is modified // in-place. @@ -206,6 +208,7 @@ func (i *Injector) applyAspects(decorator *decorator.Decorator, file *dst.File, RefMap: &references, SourceParser: decorator, MinGoLang: &minGoLang, + TestMain: i.TestMain, }) defer ctx.Release() changed, err = i.injectNode(ctx) diff --git a/internal/toolexec/aspect/oncompile.go b/internal/toolexec/aspect/oncompile.go index 8868b577..7917aa4b 100644 --- a/internal/toolexec/aspect/oncompile.go +++ b/internal/toolexec/aspect/oncompile.go @@ -129,6 +129,7 @@ func (w Weaver) OnCompile(cmd *proxy.CompileCommand) (err error) { RootConfig: map[string]string{"httpmode": "wrap"}, Lookup: imports.Lookup, ImportPath: w.ImportPath, + TestMain: cmd.TestMain() && strings.HasSuffix(w.ImportPath, ".test"), GoVersion: cmd.Flags.Lang, ModifiedFile: func(file string) string { return filepath.Join(orchestrionDir, "src", cmd.Flags.Package, filepath.Base(file)) diff --git a/internal/toolexec/proxy/compile.go b/internal/toolexec/proxy/compile.go index b58c5906..7667d291 100644 --- a/internal/toolexec/proxy/compile.go +++ b/internal/toolexec/proxy/compile.go @@ -38,6 +38,26 @@ func (c *CompileCommand) ShowVersion() bool { return c.Flags.ShowVersion } +// TestMain returns true if the compiled package name is "main" and all source +// Go files are rooted in the same directory as the importcfg file. This +// indicates the package being compiled is a synthetic "main" package generated +// by `go test`. For more accurate readings, users should also validate the +// declared package import path ends in `.test`. +func (c *CompileCommand) TestMain() bool { + if c.Flags.Package != "main" { + return false + } + + stageDir := filepath.Dir(c.Flags.ImportCfg) + for _, f := range c.GoFiles() { + if filepath.Dir(f) != stageDir { + return false + } + } + + return true +} + func (cmd *CompileCommand) SetLang(to context.GoLangVersion) error { if to.IsAny() { // No minimal language requirement change, nothing to do...