From c870fcaf19f4e5a22cc31a1381b63793c2ad2ebd Mon Sep 17 00:00:00 2001 From: Daniel Redondo Date: Thu, 23 Jan 2020 15:00:39 +0100 Subject: [PATCH 1/9] initial --- instrumentation/testing/init.go | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/instrumentation/testing/init.go b/instrumentation/testing/init.go index 58cda8e6..89c21f9f 100644 --- a/instrumentation/testing/init.go +++ b/instrumentation/testing/init.go @@ -2,9 +2,13 @@ package testing import ( "flag" + "os" "reflect" + "runtime" "testing" + "github.com/undefinedlabs/go-mpatch" + "go.undefinedlabs.com/scopeagent/instrumentation" "go.undefinedlabs.com/scopeagent/reflection" ) @@ -56,4 +60,28 @@ func Init(m *testing.M) { } *intBenchmarks = benchmarks } + + if envDMPatch, set := os.LookupEnv("SCOPE_DISABLE_MONKEY_PATCHING"); !set || envDMPatch == "" { + // We monkey patch the `testing.T.Run()` func to auto instrument sub tests + var t *testing.T + tType := reflect.TypeOf(t) + if tRunMethod, ok := tType.MethodByName("Run"); ok { + var runPatch *mpatch.Patch + var err error + runPatch, err = mpatch.PatchMethodByReflect(tRunMethod, func(t *testing.T, name string, f func(t *testing.T)) bool { + logOnError(runPatch.Unpatch()) + defer func() { + logOnError(runPatch.Patch()) + }() + pc, _, _, _ := runtime.Caller(1) + return t.Run(name, func(childT *testing.T) { + addAutoInstrumentedTest(childT) + childTest := StartTestFromCaller(childT, pc) + defer childTest.end() + f(childT) + }) + }) + logOnError(err) + } + } } From f5070029b17d8d08586b59b11cb8d214d4727dce Mon Sep 17 00:00:00 2001 From: Daniel Redondo Date: Thu, 23 Jan 2020 15:42:48 +0100 Subject: [PATCH 2/9] initial 2 --- instrumentation/testing/init.go | 6 +++--- instrumentation/testing/logger.go | 2 ++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/instrumentation/testing/init.go b/instrumentation/testing/init.go index 89c21f9f..672a026c 100644 --- a/instrumentation/testing/init.go +++ b/instrumentation/testing/init.go @@ -70,11 +70,11 @@ func Init(m *testing.M) { var err error runPatch, err = mpatch.PatchMethodByReflect(tRunMethod, func(t *testing.T, name string, f func(t *testing.T)) bool { logOnError(runPatch.Unpatch()) - defer func() { - logOnError(runPatch.Patch()) - }() + defer func() { logOnError(runPatch.Patch()) }() pc, _, _, _ := runtime.Caller(1) return t.Run(name, func(childT *testing.T) { + logOnError(runPatch.Patch()) + defer func() { logOnError(runPatch.Unpatch()) }() addAutoInstrumentedTest(childT) childTest := StartTestFromCaller(childT, pc) defer childTest.end() diff --git a/instrumentation/testing/logger.go b/instrumentation/testing/logger.go index 7603de81..579b3266 100644 --- a/instrumentation/testing/logger.go +++ b/instrumentation/testing/logger.go @@ -1,6 +1,7 @@ package testing import ( + "fmt" "reflect" "sync" "testing" @@ -173,6 +174,7 @@ func patch(methodName string, methodBody func(test *Test, argsValues []interface func logOnError(err error) { if err != nil { instrumentation.Logger().Println(err) + fmt.Println(err) } } From 5f8e5d3f9bb38be32aaba867b23e9304012d2209 Mon Sep 17 00:00:00 2001 From: Daniel Redondo Date: Fri, 24 Jan 2020 17:12:39 +0100 Subject: [PATCH 3/9] changes --- instrumentation/testing/init.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/instrumentation/testing/init.go b/instrumentation/testing/init.go index 672a026c..6d3041d5 100644 --- a/instrumentation/testing/init.go +++ b/instrumentation/testing/init.go @@ -69,12 +69,11 @@ func Init(m *testing.M) { var runPatch *mpatch.Patch var err error runPatch, err = mpatch.PatchMethodByReflect(tRunMethod, func(t *testing.T, name string, f func(t *testing.T)) bool { - logOnError(runPatch.Unpatch()) - defer func() { logOnError(runPatch.Patch()) }() pc, _, _, _ := runtime.Caller(1) + logOnError(runPatch.Unpatch()) + defer runPatch.Patch() return t.Run(name, func(childT *testing.T) { - logOnError(runPatch.Patch()) - defer func() { logOnError(runPatch.Unpatch()) }() + _ = runPatch.Patch() addAutoInstrumentedTest(childT) childTest := StartTestFromCaller(childT, pc) defer childTest.end() From fae783cdf67bc732edca56c18be3056bd0bf6a91 Mon Sep 17 00:00:00 2001 From: Daniel Redondo Date: Mon, 27 Jan 2020 13:23:04 +0100 Subject: [PATCH 4/9] sub test and sub benchmark auto instrumentation --- instrumentation/testing/benchmark.go | 4 +- instrumentation/testing/go_benchmark.go | 115 ++++++++++++++++++ instrumentation/testing/go_testing.go | 135 +++++++++++++++++++++ instrumentation/testing/go_testing.s | 4 + instrumentation/testing/go_testing_go13.go | 35 ++++++ instrumentation/testing/go_testing_go14.go | 36 ++++++ instrumentation/testing/init.go | 30 +++-- 7 files changed, 349 insertions(+), 10 deletions(-) create mode 100644 instrumentation/testing/go_benchmark.go create mode 100644 instrumentation/testing/go_testing.go create mode 100644 instrumentation/testing/go_testing.s create mode 100644 instrumentation/testing/go_testing_go13.go create mode 100644 instrumentation/testing/go_testing_go14.go diff --git a/instrumentation/testing/benchmark.go b/instrumentation/testing/benchmark.go index a704f580..cbc4bd86 100644 --- a/instrumentation/testing/benchmark.go +++ b/instrumentation/testing/benchmark.go @@ -43,7 +43,7 @@ func StartBenchmark(b *testing.B, pc uintptr, benchFunc func(b *testing.B)) { // Runs an auto instrumented sub benchmark func (bench *Benchmark) Run(name string, f func(b *testing.B)) bool { pc, _, _, _ := runtime.Caller(1) - return bench.b.Run(name, func(innerB *testing.B) { + return FromTestingB(bench.b).Run(name, func(innerB *testing.B) { startBenchmark(innerB, pc, f) }) } @@ -78,7 +78,7 @@ func startBenchmark(b *testing.B, pc uintptr, benchFunc func(b *testing.B)) { b.ReportAllocs() b.ResetTimer() startTime := time.Now() - result := b.Run("*&", func(b1 *testing.B) { + result := FromTestingB(b).Run("*&", func(b1 *testing.B) { addBenchmark(b1, &Benchmark{b: b1}) benchFunc(b1) bChild = b1 diff --git a/instrumentation/testing/go_benchmark.go b/instrumentation/testing/go_benchmark.go new file mode 100644 index 00000000..8a837db0 --- /dev/null +++ b/instrumentation/testing/go_benchmark.go @@ -0,0 +1,115 @@ +/* + The purpose with this file is to clone the struct alignment of the testing.B struct so we can assign a *testing.B + pointer to the *goB to have access to the internal private fields. + + We use this to create a Run clone method to be called from the subtest auto instrumentation +*/ + +package testing + +import ( + "runtime" + "sync" + "sync/atomic" + "testing" + "time" + "unsafe" +) + +// clone of testing.B struct +type goB struct { + goCommon + importPath string + context *goBenchContext + N int + previousN int + previousDuration time.Duration + benchFunc func(b *testing.B) + benchTime goBenchTimeFlag + bytes int64 + missingBytes bool + timerOn bool + showAllocResult bool + result testing.BenchmarkResult + parallelism int + startAllocs uint64 + startBytes uint64 + netAllocs uint64 + netBytes uint64 + extra map[string]float64 +} + +// clone of testing.benchContext struct +type goBenchContext struct { + match *goMatcher + maxLen int + extLen int +} + +// clone of testing.benchTimeFlag struct +type goBenchTimeFlag struct { + d time.Duration + n int +} + +// Convert *goB to *testing.B +func (b *goB) ToTestingB() *testing.B { + return *(**testing.B)(unsafe.Pointer(&b)) +} + +// Convert *testing.B to *goB +func FromTestingB(b *testing.B) *goB { + return *(**goB)(unsafe.Pointer(&b)) +} + +//go:linkname benchmarkLock testing.benchmarkLock +var benchmarkLock sync.Mutex + +//go:linkname (*goB).run1 testing.(*B).run1 +func (b *goB) run1() bool + +//go:linkname (*goB).run testing.(*B).run +func (b *goB) run() bool + +//go:linkname (*goB).add testing.(*B).add +func (b *goB) add(other testing.BenchmarkResult) + +// we clone the same (*testing.B).Run implementation because the Patch overwrite the original implementation with the jump +func (b *goB) Run(name string, f func(b *testing.B)) bool { + atomic.StoreInt32(&b.hasSub, 1) + benchmarkLock.Unlock() + defer benchmarkLock.Lock() + + benchName, ok, partial := b.name, true, false + if b.context != nil { + benchName, ok, partial = b.context.match.fullName(&b.goCommon, name) + } + if !ok { + return true + } + var pc [maxStackLen]uintptr + n := runtime.Callers(2, pc[:]) + sub := &goB{ + goCommon: goCommon{ + signal: make(chan bool), + name: benchName, + parent: &b.goCommon, + level: b.level + 1, + creator: pc[:n], + w: b.w, + chatty: b.chatty, + }, + importPath: b.importPath, + benchFunc: f, + benchTime: b.benchTime, + context: b.context, + } + if partial { + atomic.StoreInt32(&sub.hasSub, 1) + } + if sub.run1() { + sub.run() + } + b.add(sub.result) + return !sub.failed +} diff --git a/instrumentation/testing/go_testing.go b/instrumentation/testing/go_testing.go new file mode 100644 index 00000000..74ec3f2e --- /dev/null +++ b/instrumentation/testing/go_testing.go @@ -0,0 +1,135 @@ +/* + The purpose with this file is to clone the struct alignment of the testing.T struct so we can assign a *testing.T + pointer to the *goT to have access to the internal private fields. + + We use this to create a Run clone method to be called from the subtest auto instrumentation +*/ +package testing + +import ( + "bytes" + "fmt" + "runtime" + "sync" + "sync/atomic" + "testing" + "unsafe" +) + +// clone of testing.T struct +type goT struct { + goCommon + isParallel bool + context *goTestContext +} + +// clone of testing.testContext struct +type goTestContext struct { + match *goMatcher + mu sync.Mutex + startParallel chan bool + running int + numWaiting int + maxParallel int +} + +// clone of testing.matcher struct +type goMatcher struct { + filter []string + matchFunc func(pat, str string) (bool, error) + mu sync.Mutex + subNames map[string]int64 +} + +// clone of testing.indenter struct +type goIndenter struct { + c *goCommon +} + +// Convert *goT to *testing.T +func (t *goT) ToTestingT() *testing.T { + return *(**testing.T)(unsafe.Pointer(&t)) +} + +// Convert *testing.T to *goT +func FromTestingT(t *testing.T) *goT { + return *(**goT)(unsafe.Pointer(&t)) +} + +const maxStackLen = 50 + +//go:linkname matchMutex testing.matchMutex +var matchMutex sync.Mutex + +//go:linkname goTRunner testing.tRunner +func goTRunner(t *testing.T, fn func(t *testing.T)) + +//go:linkname rewrite testing.rewrite +func rewrite(s string) string + +//go:linkname shouldFailFast testing.shouldFailFast +func shouldFailFast() bool + +//go:linkname (*goMatcher).fullName testing.(*matcher).fullName +func (m *goMatcher) fullName(c *goCommon, subname string) (name string, ok, partial bool) + +// this method calls the original testing.tRunner by converting *goT to *testing.T +func tRunner(t *goT, fn func(t *goT)) { + goTRunner(t.ToTestingT(), func(t *testing.T) { fn(FromTestingT(t)) }) +} + +// we clone the same (*testing.T).Run implementation because the Patch overwrite the original implementation with the jump +func (t *goT) Run(name string, f func(t *goT)) bool { + atomic.StoreInt32(&t.hasSub, 1) + testName, ok, _ := t.context.match.fullName(&t.goCommon, name) + if !ok || shouldFailFast() { + return true + } + var pc [maxStackLen]uintptr + n := runtime.Callers(2, pc[:]) + t = &goT{ + goCommon: goCommon{ + barrier: make(chan bool), + signal: make(chan bool), + name: testName, + parent: &t.goCommon, + level: t.level + 1, + creator: pc[:n], + chatty: t.chatty, + }, + context: t.context, + } + t.w = goIndenter{&t.goCommon} + + if t.chatty { + root := t.parent + for ; root.parent != nil; root = root.parent { + } + root.mu.Lock() + fmt.Fprintf(root.w, "=== RUN %s\n", t.name) + root.mu.Unlock() + } + go tRunner(t, f) + if !<-t.signal { + runtime.Goexit() + } + return !t.failed +} + +// we can't link an instance method without a struct pointer +func (w goIndenter) Write(b []byte) (n int, err error) { + n = len(b) + for len(b) > 0 { + end := bytes.IndexByte(b, '\n') + if end == -1 { + end = len(b) + } else { + end++ + } + const indent = " " + w.c.output = append(w.c.output, indent...) + w.c.output = append(w.c.output, b[:end]...) + b = b[end:] + } + return +} diff --git a/instrumentation/testing/go_testing.s b/instrumentation/testing/go_testing.s new file mode 100644 index 00000000..568c2394 --- /dev/null +++ b/instrumentation/testing/go_testing.s @@ -0,0 +1,4 @@ +// The testing package uses //go:linkname to push a few functions into this +// package but we still need a .s file so the Go tool does not pass -complete +// to the go tool compile so the latter does not complain about Go functions +// with no bodies. \ No newline at end of file diff --git a/instrumentation/testing/go_testing_go13.go b/instrumentation/testing/go_testing_go13.go new file mode 100644 index 00000000..82bef62a --- /dev/null +++ b/instrumentation/testing/go_testing_go13.go @@ -0,0 +1,35 @@ +// +build !go1.14 + +package testing + +import ( + "io" + "sync" + "time" +) + +// clone of testing.common struct +type goCommon struct { + mu sync.RWMutex + output []byte + w io.Writer + ran bool + failed bool + skipped bool + done bool + helpers map[string]struct{} + chatty bool + finished bool + hasSub int32 + raceErrors int + runner string + parent *goCommon + level int + creator []uintptr + name string + start time.Time + duration time.Duration + barrier chan bool + signal chan bool + sub []*goT +} diff --git a/instrumentation/testing/go_testing_go14.go b/instrumentation/testing/go_testing_go14.go new file mode 100644 index 00000000..f59a8a99 --- /dev/null +++ b/instrumentation/testing/go_testing_go14.go @@ -0,0 +1,36 @@ +// +build go1.14 + +package testing + +import ( + "io" + "sync" + "time" +) + +// clone of testing.common struct +type goCommon struct { + mu sync.RWMutex + output []byte + w io.Writer + ran bool + failed bool + skipped bool + done bool + helpers map[string]struct{} + cleanup func() // New in golang 1.14 + chatty bool + finished bool + hasSub int32 + raceErrors int + runner string + parent *goCommon + level int + creator []uintptr + name string + start time.Time + duration time.Duration + barrier chan bool + signal chan bool + sub []*goT +} diff --git a/instrumentation/testing/init.go b/instrumentation/testing/init.go index 6d3041d5..20b2656d 100644 --- a/instrumentation/testing/init.go +++ b/instrumentation/testing/init.go @@ -54,7 +54,11 @@ func Init(m *testing.M) { benchmarks = append(benchmarks, testing.InternalBenchmark{ Name: benchmark.Name, F: func(b *testing.B) { // Indirection of the original benchmark - startBenchmark(b, funcPointer, funcValue) + if envDMPatch, set := os.LookupEnv("SCOPE_DISABLE_MONKEY_PATCHING"); !set || envDMPatch == "" { + funcValue(b) + } else { + startBenchmark(b, funcPointer, funcValue) + } }, }) } @@ -66,14 +70,11 @@ func Init(m *testing.M) { var t *testing.T tType := reflect.TypeOf(t) if tRunMethod, ok := tType.MethodByName("Run"); ok { - var runPatch *mpatch.Patch - var err error - runPatch, err = mpatch.PatchMethodByReflect(tRunMethod, func(t *testing.T, name string, f func(t *testing.T)) bool { + _, err := mpatch.PatchMethodByReflect(tRunMethod, func(t *testing.T, name string, f func(t *testing.T)) bool { pc, _, _, _ := runtime.Caller(1) - logOnError(runPatch.Unpatch()) - defer runPatch.Patch() - return t.Run(name, func(childT *testing.T) { - _ = runPatch.Patch() + gT := FromTestingT(t) + return gT.Run(name, func(childGoT *goT) { + childT := childGoT.ToTestingT() addAutoInstrumentedTest(childT) childTest := StartTestFromCaller(childT, pc) defer childTest.end() @@ -82,5 +83,18 @@ func Init(m *testing.M) { }) logOnError(err) } + + // We monkey patch the `testing.B.Run()` func to auto instrument sub benchmark + var b *testing.B + bType := reflect.TypeOf(b) + if bRunMethod, ok := bType.MethodByName("Run"); ok { + _, err := mpatch.PatchMethodByReflect(bRunMethod, func(b *testing.B, name string, f func(b *testing.B)) bool { + pc, _, _, _ := runtime.Caller(1) + return FromTestingB(b).Run(name, func(b *testing.B) { + StartBenchmark(b, pc, f) + }) + }) + logOnError(err) + } } } From 818bc29edc694f97b28aacdcbbd0def2462fd9f0 Mon Sep 17 00:00:00 2001 From: Daniel Redondo Date: Mon, 27 Jan 2020 13:40:10 +0100 Subject: [PATCH 5/9] sub test and sub benchmark auto instrumentation --- instrumentation/testing/go_benchmark.go | 3 ++- instrumentation/testing/go_testing.go | 3 ++- instrumentation/testing/logger.go | 1 - 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/instrumentation/testing/go_benchmark.go b/instrumentation/testing/go_benchmark.go index 8a837db0..ec6b032e 100644 --- a/instrumentation/testing/go_benchmark.go +++ b/instrumentation/testing/go_benchmark.go @@ -74,7 +74,8 @@ func (b *goB) run() bool //go:linkname (*goB).add testing.(*B).add func (b *goB) add(other testing.BenchmarkResult) -// we clone the same (*testing.B).Run implementation because the Patch overwrite the original implementation with the jump +// we clone the same (*testing.B).Run implementation because the Patch +// overwrites the original implementation with the jump func (b *goB) Run(name string, f func(b *testing.B)) bool { atomic.StoreInt32(&b.hasSub, 1) benchmarkLock.Unlock() diff --git a/instrumentation/testing/go_testing.go b/instrumentation/testing/go_testing.go index 74ec3f2e..fc56b82a 100644 --- a/instrumentation/testing/go_testing.go +++ b/instrumentation/testing/go_testing.go @@ -78,7 +78,8 @@ func tRunner(t *goT, fn func(t *goT)) { goTRunner(t.ToTestingT(), func(t *testing.T) { fn(FromTestingT(t)) }) } -// we clone the same (*testing.T).Run implementation because the Patch overwrite the original implementation with the jump +// we clone the same (*testing.T).Run implementation because the Patch +// overwrites the original implementation with the jump func (t *goT) Run(name string, f func(t *goT)) bool { atomic.StoreInt32(&t.hasSub, 1) testName, ok, _ := t.context.match.fullName(&t.goCommon, name) diff --git a/instrumentation/testing/logger.go b/instrumentation/testing/logger.go index 579b3266..13fdf59e 100644 --- a/instrumentation/testing/logger.go +++ b/instrumentation/testing/logger.go @@ -174,7 +174,6 @@ func patch(methodName string, methodBody func(test *Test, argsValues []interface func logOnError(err error) { if err != nil { instrumentation.Logger().Println(err) - fmt.Println(err) } } From c3062cfe243836a89d43e9268737954fe6a8b374 Mon Sep 17 00:00:00 2001 From: Daniel Redondo Date: Mon, 27 Jan 2020 13:40:52 +0100 Subject: [PATCH 6/9] sub test and sub benchmark auto instrumentation --- instrumentation/testing/logger.go | 1 - 1 file changed, 1 deletion(-) diff --git a/instrumentation/testing/logger.go b/instrumentation/testing/logger.go index 13fdf59e..7603de81 100644 --- a/instrumentation/testing/logger.go +++ b/instrumentation/testing/logger.go @@ -1,7 +1,6 @@ package testing import ( - "fmt" "reflect" "sync" "testing" From b913be294b73204d7ea61b9449639ccbac06d7f7 Mon Sep 17 00:00:00 2001 From: Daniel Redondo Date: Mon, 27 Jan 2020 14:14:17 +0100 Subject: [PATCH 7/9] sub test and sub benchmark auto instrumentation --- instrumentation/testing/go_benchmark.go | 3 ++- instrumentation/testing/go_testing.go | 16 ++++++---------- instrumentation/testing/init.go | 3 +-- 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/instrumentation/testing/go_benchmark.go b/instrumentation/testing/go_benchmark.go index ec6b032e..c090946e 100644 --- a/instrumentation/testing/go_benchmark.go +++ b/instrumentation/testing/go_benchmark.go @@ -2,7 +2,8 @@ The purpose with this file is to clone the struct alignment of the testing.B struct so we can assign a *testing.B pointer to the *goB to have access to the internal private fields. - We use this to create a Run clone method to be called from the subtest auto instrumentation + We use this to create a Run clone method to be called from the sub benchmark auto instrumentation (because the original + method is replaced with the Patch) */ package testing diff --git a/instrumentation/testing/go_testing.go b/instrumentation/testing/go_testing.go index fc56b82a..5086c0af 100644 --- a/instrumentation/testing/go_testing.go +++ b/instrumentation/testing/go_testing.go @@ -2,7 +2,8 @@ The purpose with this file is to clone the struct alignment of the testing.T struct so we can assign a *testing.T pointer to the *goT to have access to the internal private fields. - We use this to create a Run clone method to be called from the subtest auto instrumentation + We use this to create a Run clone method to be called from the sub test auto instrumentation (because the original + method is replaced with the Patch) */ package testing @@ -61,8 +62,8 @@ const maxStackLen = 50 //go:linkname matchMutex testing.matchMutex var matchMutex sync.Mutex -//go:linkname goTRunner testing.tRunner -func goTRunner(t *testing.T, fn func(t *testing.T)) +//go:linkname tRunner testing.tRunner +func tRunner(t *testing.T, fn func(t *testing.T)) //go:linkname rewrite testing.rewrite func rewrite(s string) string @@ -73,14 +74,9 @@ func shouldFailFast() bool //go:linkname (*goMatcher).fullName testing.(*matcher).fullName func (m *goMatcher) fullName(c *goCommon, subname string) (name string, ok, partial bool) -// this method calls the original testing.tRunner by converting *goT to *testing.T -func tRunner(t *goT, fn func(t *goT)) { - goTRunner(t.ToTestingT(), func(t *testing.T) { fn(FromTestingT(t)) }) -} - // we clone the same (*testing.T).Run implementation because the Patch // overwrites the original implementation with the jump -func (t *goT) Run(name string, f func(t *goT)) bool { +func (t *goT) Run(name string, f func(t *testing.T)) bool { atomic.StoreInt32(&t.hasSub, 1) testName, ok, _ := t.context.match.fullName(&t.goCommon, name) if !ok || shouldFailFast() { @@ -110,7 +106,7 @@ func (t *goT) Run(name string, f func(t *goT)) bool { fmt.Fprintf(root.w, "=== RUN %s\n", t.name) root.mu.Unlock() } - go tRunner(t, f) + go tRunner(t.ToTestingT(), f) if !<-t.signal { runtime.Goexit() } diff --git a/instrumentation/testing/init.go b/instrumentation/testing/init.go index 20b2656d..bb036931 100644 --- a/instrumentation/testing/init.go +++ b/instrumentation/testing/init.go @@ -73,8 +73,7 @@ func Init(m *testing.M) { _, err := mpatch.PatchMethodByReflect(tRunMethod, func(t *testing.T, name string, f func(t *testing.T)) bool { pc, _, _, _ := runtime.Caller(1) gT := FromTestingT(t) - return gT.Run(name, func(childGoT *goT) { - childT := childGoT.ToTestingT() + return gT.Run(name, func(childT *testing.T) { addAutoInstrumentedTest(childT) childTest := StartTestFromCaller(childT, pc) defer childTest.end() From 700e0ce972639dc660657f52deeb689159563247 Mon Sep 17 00:00:00 2001 From: Daniel Redondo Date: Thu, 20 Feb 2020 11:34:36 +0100 Subject: [PATCH 8/9] changes --- autoinstrument/init.go | 50 +++++++++++-------- instrumentation/testing/init.go | 79 ++++++++++++++++++------------- instrumentation/testing/logger.go | 7 +-- 3 files changed, 80 insertions(+), 56 deletions(-) diff --git a/autoinstrument/init.go b/autoinstrument/init.go index 17abcde4..988e2479 100644 --- a/autoinstrument/init.go +++ b/autoinstrument/init.go @@ -19,31 +19,39 @@ var ( func init() { once.Do(func() { - var m *testing.M - var mRunMethod reflect.Method - var ok bool - mType := reflect.TypeOf(m) - if mRunMethod, ok = mType.MethodByName("Run"); !ok { - return - } - - var runPatch *mpatch.Patch - var err error - runPatch, err = mpatch.PatchMethodByReflect(mRunMethod, func(m *testing.M) int { - logOnError(runPatch.Unpatch()) - defer func() { - logOnError(runPatch.Patch()) - }() - scopetesting.PatchTestingLogger() - defer scopetesting.UnpatchTestingLogger() - return scopeagent.Run(m, agent.WithGlobalPanicHandler()) - }) - logOnError(err) + patchMRun() + scopetesting.PatchTRun() + scopetesting.PatchBRun() }) } -func logOnError(err error) { +func patchMRun() { + var m *testing.M + var mRunMethod reflect.Method + var ok bool + mType := reflect.TypeOf(m) + if mRunMethod, ok = mType.MethodByName("Run"); !ok { + return + } + + var runPatch *mpatch.Patch + var err error + runPatch, err = mpatch.PatchMethodByReflect(mRunMethod, func(m *testing.M) int { + logOnError(runPatch.Unpatch()) + defer func() { + logOnError(runPatch.Patch()) + }() + scopetesting.PatchTestingLogger() + defer scopetesting.UnpatchTestingLogger() + return scopeagent.Run(m, agent.WithGlobalPanicHandler()) + }) + logOnError(err) +} + +func logOnError(err error) bool { if err != nil { instrumentation.Logger().Println(err) + return true } + return false } diff --git a/instrumentation/testing/init.go b/instrumentation/testing/init.go index bb036931..7e4f60b4 100644 --- a/instrumentation/testing/init.go +++ b/instrumentation/testing/init.go @@ -2,7 +2,6 @@ package testing import ( "flag" - "os" "reflect" "runtime" "testing" @@ -14,7 +13,9 @@ import ( ) var ( - parallel int + parallel int + tRunPatched bool + bRunPatched bool ) // Initialize the testing instrumentation @@ -54,7 +55,7 @@ func Init(m *testing.M) { benchmarks = append(benchmarks, testing.InternalBenchmark{ Name: benchmark.Name, F: func(b *testing.B) { // Indirection of the original benchmark - if envDMPatch, set := os.LookupEnv("SCOPE_DISABLE_MONKEY_PATCHING"); !set || envDMPatch == "" { + if bRunPatched { funcValue(b) } else { startBenchmark(b, funcPointer, funcValue) @@ -64,36 +65,50 @@ func Init(m *testing.M) { } *intBenchmarks = benchmarks } +} - if envDMPatch, set := os.LookupEnv("SCOPE_DISABLE_MONKEY_PATCHING"); !set || envDMPatch == "" { - // We monkey patch the `testing.T.Run()` func to auto instrument sub tests - var t *testing.T - tType := reflect.TypeOf(t) - if tRunMethod, ok := tType.MethodByName("Run"); ok { - _, err := mpatch.PatchMethodByReflect(tRunMethod, func(t *testing.T, name string, f func(t *testing.T)) bool { - pc, _, _, _ := runtime.Caller(1) - gT := FromTestingT(t) - return gT.Run(name, func(childT *testing.T) { - addAutoInstrumentedTest(childT) - childTest := StartTestFromCaller(childT, pc) - defer childTest.end() - f(childT) - }) - }) - logOnError(err) - } +func PatchTRun() { + // We monkey patch the `testing.T.Run()` func to auto instrument sub tests + var t *testing.T + var tRunMethod reflect.Method + var ok bool + tType := reflect.TypeOf(t) + if tRunMethod, ok = tType.MethodByName("Run"); !ok { + return + } - // We monkey patch the `testing.B.Run()` func to auto instrument sub benchmark - var b *testing.B - bType := reflect.TypeOf(b) - if bRunMethod, ok := bType.MethodByName("Run"); ok { - _, err := mpatch.PatchMethodByReflect(bRunMethod, func(b *testing.B, name string, f func(b *testing.B)) bool { - pc, _, _, _ := runtime.Caller(1) - return FromTestingB(b).Run(name, func(b *testing.B) { - StartBenchmark(b, pc, f) - }) - }) - logOnError(err) - } + _, err := mpatch.PatchMethodByReflect(tRunMethod, func(t *testing.T, name string, f func(t *testing.T)) bool { + pc, _, _, _ := runtime.Caller(1) + gT := FromTestingT(t) + return gT.Run(name, func(childT *testing.T) { + addAutoInstrumentedTest(childT) + childTest := StartTestFromCaller(childT, pc) + defer childTest.end() + f(childT) + }) + }) + if !logOnError(err) { + tRunPatched = true + } +} + +func PatchBRun() { + // We monkey patch the `testing.B.Run()` func to auto instrument sub benchmark + var b *testing.B + var bRunMethod reflect.Method + var ok bool + bType := reflect.TypeOf(b) + if bRunMethod, ok = bType.MethodByName("Run"); !ok { + return + } + + _, err := mpatch.PatchMethodByReflect(bRunMethod, func(b *testing.B, name string, f func(b *testing.B)) bool { + pc, _, _, _ := runtime.Caller(1) + return FromTestingB(b).Run(name, func(b *testing.B) { + StartBenchmark(b, pc, f) + }) + }) + if !logOnError(err) { + bRunPatched = true } } diff --git a/instrumentation/testing/logger.go b/instrumentation/testing/logger.go index 7603de81..cf9cfd62 100644 --- a/instrumentation/testing/logger.go +++ b/instrumentation/testing/logger.go @@ -163,17 +163,18 @@ func patch(methodName string, methodBody func(test *Test, argsValues []interface methodBody(test, argIn) return nil }) - logOnError(err) - if err == nil { + if !logOnError(err) { patches[methodName] = methodPatch patchPointers[reflect.ValueOf(methodBody).Pointer()] = true } } -func logOnError(err error) { +func logOnError(err error) bool { if err != nil { instrumentation.Logger().Println(err) + return true } + return false } func isAPatchPointer(ptr uintptr) bool { From 36da140de58f66b15ebcbac11850fa560b55e234 Mon Sep 17 00:00:00 2001 From: Daniel Redondo Date: Thu, 19 Mar 2020 10:54:31 +0100 Subject: [PATCH 9/9] fix subtest caller pc --- instrumentation/testing/init.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/instrumentation/testing/init.go b/instrumentation/testing/init.go index 7e4f60b4..af4e3e79 100644 --- a/instrumentation/testing/init.go +++ b/instrumentation/testing/init.go @@ -3,7 +3,6 @@ package testing import ( "flag" "reflect" - "runtime" "testing" "github.com/undefinedlabs/go-mpatch" @@ -78,7 +77,7 @@ func PatchTRun() { } _, err := mpatch.PatchMethodByReflect(tRunMethod, func(t *testing.T, name string, f func(t *testing.T)) bool { - pc, _, _, _ := runtime.Caller(1) + pc := reflect.ValueOf(f).Pointer() gT := FromTestingT(t) return gT.Run(name, func(childT *testing.T) { addAutoInstrumentedTest(childT) @@ -103,7 +102,7 @@ func PatchBRun() { } _, err := mpatch.PatchMethodByReflect(bRunMethod, func(b *testing.B, name string, f func(b *testing.B)) bool { - pc, _, _, _ := runtime.Caller(1) + pc := reflect.ValueOf(f).Pointer() return FromTestingB(b).Run(name, func(b *testing.B) { StartBenchmark(b, pc, f) })