From 0e5088665489b907dc4754c85ce31f52e438b4a3 Mon Sep 17 00:00:00 2001 From: Piotr WOLSKI Date: Mon, 9 Aug 2021 20:15:23 -0400 Subject: [PATCH 1/8] .circleci: Add check for grouping imports (#795) This adds a circleci stage to ensure imports are grouped correctly. --- .circleci/config.yml | 7 +++++++ ddtrace/tracer/context_test.go | 1 + ddtrace/tracer/spancontext_test.go | 1 + ddtrace/tracer/writer_test.go | 1 + 4 files changed, 10 insertions(+) diff --git a/.circleci/config.yml b/.circleci/config.yml index 1fb6f99290..60b3876f4a 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -60,6 +60,13 @@ jobs: exit 1 fi + - run: + name: goimports + command: | + if [ "$(goimports -e -l -local gopkg.in/DataDog/dd-trace-go.v1 . | wc -l)" -gt 0 ]; then + exit 1 + fi + - run: name: lint command: | diff --git a/ddtrace/tracer/context_test.go b/ddtrace/tracer/context_test.go index 70b5ee33bb..ab6ad3b53f 100644 --- a/ddtrace/tracer/context_test.go +++ b/ddtrace/tracer/context_test.go @@ -10,6 +10,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/internal" ) diff --git a/ddtrace/tracer/spancontext_test.go b/ddtrace/tracer/spancontext_test.go index da0c4ae3c8..38670419e7 100644 --- a/ddtrace/tracer/spancontext_test.go +++ b/ddtrace/tracer/spancontext_test.go @@ -12,6 +12,7 @@ import ( "time" "github.com/stretchr/testify/assert" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/internal/log" ) diff --git a/ddtrace/tracer/writer_test.go b/ddtrace/tracer/writer_test.go index 1a044217d4..69d1522a17 100644 --- a/ddtrace/tracer/writer_test.go +++ b/ddtrace/tracer/writer_test.go @@ -14,6 +14,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "gopkg.in/DataDog/dd-trace-go.v1/internal/log" ) From 394c515a0fb3fa9de2194ea145176ae94219fba0 Mon Sep 17 00:00:00 2001 From: Gabriel Aszalos Date: Fri, 13 Aug 2021 09:19:08 +0300 Subject: [PATCH 2/8] ddtrace/tracer: remove the waitClose mechanism (#976) This change removes the `(*payload).waitClose` mechanism added in #475 because it is no longer necessary since #549, where we've stopped reusing payloads and started sending them async. The change also removes the `(*payload).reset` method implementation to further emphasise that this type of use is discouraged. Additionally, the Close call now resets the buffer to ensure it is garbage collected after use, regardless of still being referenced or not. See also https://github.com/golang/go/blob/go1.16/src/net/http/client.go#L136-L138 --- ddtrace/tracer/payload.go | 48 ++++++++++++++------------------ ddtrace/tracer/payload_test.go | 17 +++++++---- ddtrace/tracer/transport.go | 1 - ddtrace/tracer/transport_test.go | 36 ------------------------ ddtrace/tracer/writer.go | 5 ++-- 5 files changed, 35 insertions(+), 72 deletions(-) diff --git a/ddtrace/tracer/payload.go b/ddtrace/tracer/payload.go index 904ca8cb8f..2107ac0635 100644 --- a/ddtrace/tracer/payload.go +++ b/ddtrace/tracer/payload.go @@ -24,11 +24,8 @@ import ( // payload implements io.Reader and can be used with the decoder directly. To create // a new payload use the newPayload method. // -// payload is not safe for concurrent use. -// -// This structure basically allows us to push traces into the payload one at a time -// in order to always have knowledge of the payload size, but also making it possible -// for the agent to decode it as an array. +// payload is not safe for concurrent use, is meant to be used only once and eventually +// dismissed. type payload struct { // header specifies the first few bytes in the msgpack stream // indicating the type of array (fixarray, array16 or array32) @@ -43,9 +40,6 @@ type payload struct { // buf holds the sequence of msgpack-encoded items. buf bytes.Buffer - - // closed specifies the notification channel for each Close call. - closed chan struct{} } var _ io.Reader = (*payload)(nil) @@ -55,7 +49,6 @@ func newPayload() *payload { p := &payload{ header: make([]byte, 8), off: 8, - closed: make(chan struct{}, 1), } return p } @@ -81,16 +74,22 @@ func (p *payload) size() int { return p.buf.Len() + len(p.header) - p.off } -// reset resets the internal buffer, counter and read offset. +// reset should *not* be used. It is not implemented and is only here to serve +// as information on how to implement it in case the same payload object ever +// needs to be reused. func (p *payload) reset() { - p.off = 8 - atomic.StoreUint64(&p.count, 0) - p.buf.Reset() - select { - case <-p.closed: - // ensure there is room - default: - } + // ⚠️ Warning! + // + // Resetting the payload for re-use requires the transport to wait for the + // HTTP package to Close the request body before attempting to re-use it + // again! This requires additional logic to be in place. See: + // + // • https://github.com/golang/go/blob/go1.16/src/net/http/client.go#L136-L138 + // • https://github.com/DataDog/dd-trace-go/pull/475 + // • https://github.com/DataDog/dd-trace-go/pull/549 + // • https://github.com/DataDog/dd-trace-go/pull/976 + // + panic("not implemented") } // https://github.com/msgpack/msgpack/blob/master/spec.md#array-format-family @@ -121,18 +120,13 @@ func (p *payload) updateHeader() { // Close implements io.Closer func (p *payload) Close() error { - select { - case p.closed <- struct{}{}: - default: - // ignore subsequent Close calls - } + // Once the payload has been read, clear the buffer for garbage collection to avoid + // a memory leak when references to this object may still be kept by faulty transport + // implementations or the standard library. See dd-trace-go#976 + p.buf = bytes.Buffer{} return nil } -// waitClose blocks until the first Close call occurs since the payload -// was constructed or the last reset happened. -func (p *payload) waitClose() { <-p.closed } - // Read implements io.Reader. It reads from the msgpack-encoded stream. func (p *payload) Read(b []byte) (n int, err error) { if p.off < len(p.header) { diff --git a/ddtrace/tracer/payload_test.go b/ddtrace/tracer/payload_test.go index 317bc556c1..4a878195b2 100644 --- a/ddtrace/tracer/payload_test.go +++ b/ddtrace/tracer/payload_test.go @@ -33,11 +33,10 @@ func newSpanList(n int) spanList { // the codec. func TestPayloadIntegrity(t *testing.T) { assert := assert.New(t) - p := newPayload() want := new(bytes.Buffer) for _, n := range []int{10, 1 << 10, 1 << 17} { t.Run(strconv.Itoa(n), func(t *testing.T) { - p.reset() + p := newPayload() lists := make(spanLists, n) for i := 0; i < n; i++ { list := newSpanList(i%5 + 1) @@ -61,10 +60,9 @@ func TestPayloadIntegrity(t *testing.T) { // be decoded by the codec. func TestPayloadDecode(t *testing.T) { assert := assert.New(t) - p := newPayload() for _, n := range []int{10, 1 << 10} { t.Run(strconv.Itoa(n), func(t *testing.T) { - p.reset() + p := newPayload() for i := 0; i < n; i++ { p.push(newSpanList(i%5 + 1)) } @@ -82,7 +80,8 @@ func BenchmarkPayloadThroughput(b *testing.B) { } // benchmarkPayloadThroughput benchmarks the throughput of the payload by subsequently -// pushing a trace containing count spans of approximately 10KB in size each. +// pushing a trace containing count spans of approximately 10KB in size each, until the +// payload is filled. func benchmarkPayloadThroughput(count int) func(*testing.B) { return func(b *testing.B) { p := newPayload() @@ -94,8 +93,14 @@ func benchmarkPayloadThroughput(count int) func(*testing.B) { } b.ReportAllocs() b.ResetTimer() + reset := func() { + p.header = make([]byte, 8) + p.off = 8 + p.count = 0 + p.buf.Reset() + } for i := 0; i < b.N; i++ { - p.reset() + reset() for p.size() < payloadMaxLimit { p.push(trace) } diff --git a/ddtrace/tracer/transport.go b/ddtrace/tracer/transport.go index a7dc98d51e..0994ba2d6d 100644 --- a/ddtrace/tracer/transport.go +++ b/ddtrace/tracer/transport.go @@ -165,7 +165,6 @@ func (t *httpTransport) send(p *payload) (body io.ReadCloser, err error) { if err != nil { return nil, err } - p.waitClose() if code := response.StatusCode; code >= 400 { // error, check the body for context information and // return a nice error. diff --git a/ddtrace/tracer/transport_test.go b/ddtrace/tracer/transport_test.go index 9ff287af19..a0cbaa7112 100644 --- a/ddtrace/tracer/transport_test.go +++ b/ddtrace/tracer/transport_test.go @@ -6,7 +6,6 @@ package tracer import ( - "context" "fmt" "io/ioutil" "net" @@ -17,7 +16,6 @@ import ( "strconv" "strings" "testing" - "time" "github.com/stretchr/testify/assert" ) @@ -294,37 +292,3 @@ func TestWithUDS(t *testing.T) { assert.NoError(err) assert.Len(rt.reqs, 1) } - -// TestTransportHTTPRace defines a regression tests where the request body was being -// read even after http.Client.Do returns. See golang/go#33244 -func TestTransportHTTPRace(t *testing.T) { - srv := http.Server{ - Addr: "127.0.0.1:8889", - Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - r.Body.Read(make([]byte, 4096)) - w.WriteHeader(http.StatusRequestEntityTooLarge) - }), - } - done := make(chan struct{}) - go func() { - srv.ListenAndServe() - done <- struct{}{} - }() - trans := &httpTransport{ - traceURL: "http://127.0.0.1:8889/", - client: &http.Client{}, - } - p := newPayload() - spanList := newSpanList(50) - for i := 0; i < 100; i++ { - for j := 0; j < 100; j++ { - p.push(spanList) - } - trans.send(p) - p.reset() - } - ctx, cancelFunc := context.WithTimeout(context.Background(), time.Millisecond) - defer cancelFunc() - srv.Shutdown(ctx) - <-done -} diff --git a/ddtrace/tracer/writer.go b/ddtrace/tracer/writer.go index 8faab1dfc4..58798a7a79 100644 --- a/ddtrace/tracer/writer.go +++ b/ddtrace/tracer/writer.go @@ -81,6 +81,8 @@ func (h *agentTraceWriter) flush() { } h.wg.Add(1) h.climit <- struct{}{} + oldp := h.payload + h.payload = newPayload() go func(p *payload) { defer func(start time.Time) { <-h.climit @@ -100,8 +102,7 @@ func (h *agentTraceWriter) flush() { h.config.statsd.Incr("datadog.tracer.decode_error", nil, 1) } } - }(h.payload) - h.payload = newPayload() + }(oldp) } // logWriter specifies the output target of the logTraceWriter; replaced in tests. From e3b911fb7b543ab41b51a577aae4342faf11da18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Geisend=C3=B6rfer?= Date: Fri, 13 Aug 2021 13:48:21 +0200 Subject: [PATCH 3/8] checkcopyright: accept year, year range, year list (#977) checkcopyright: accept year, year range, year list Copyright headers, if used at all, should refer to the year(s) the code was written or substantially updated. Update checkcopyright.go to accept anything that looks like a year, year range or year list rather than hard coding 2016. --- checkcopyright.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/checkcopyright.go b/checkcopyright.go index 00c9e57ab0..678b5d246b 100644 --- a/checkcopyright.go +++ b/checkcopyright.go @@ -9,18 +9,19 @@ package main import ( - "bytes" - "fmt" "io" "log" "os" "path/filepath" + "regexp" "strings" ) func main() { var missing bool - copyrightText := []byte(fmt.Sprintf("// Copyright 2016 Datadog, Inc.")) + // copyrightRegexp matches years or year ranges like "2016", "2016-2019", + // "2016,2018-2020" in the copyright header. + copyrightRegexp := regexp.MustCompile(`// Copyright 20[0-9]{2}[0-9,\-]* Datadog, Inc.`) if err := filepath.Walk(".", func(path string, info os.FileInfo, err error) error { if err != nil { return err @@ -39,7 +40,7 @@ func main() { if err != nil && err != io.EOF { return err } - if !bytes.Contains(snip, copyrightText) { + if !copyrightRegexp.Match(snip) { // report missing header missing = true log.Printf("Copyright header missing in %q.\n", path) From 8317212676534cef40ad438ca1deb71b6dfa75dd Mon Sep 17 00:00:00 2001 From: Piotr WOLSKI Date: Tue, 17 Aug 2021 05:28:42 -0400 Subject: [PATCH 4/8] .circleci: install goimports (#979) To ensure correct grouping of imports (not order though). --- .circleci/config.yml | 5 ++++- contrib/Shopify/sarama/example_test.go | 1 + contrib/aws/aws-sdk-go-v2/aws/example_test.go | 1 + contrib/aws/aws-sdk-go/aws/aws_test.go | 1 + contrib/aws/aws-sdk-go/aws/example_test.go | 1 + contrib/bradfitz/gomemcache/memcache/example_test.go | 1 + contrib/bradfitz/gomemcache/memcache/memcache.go | 1 + contrib/bradfitz/gomemcache/memcache/memcache_test.go | 1 + .../confluent-kafka-go/kafka/example_test.go | 1 + .../confluent-kafka-go/kafka/option_test.go | 1 + contrib/database/sql/example_test.go | 1 + contrib/database/sql/internal/dsn_test.go | 1 + contrib/emicklei/go-restful/example_test.go | 1 + contrib/emicklei/go-restful/restful_test.go | 1 + contrib/gin-gonic/gin/option.go | 1 + contrib/globalsign/mgo/mgo_test.go | 1 + contrib/globalsign/mgo/pipe.go | 1 + contrib/go-redis/redis.v7/example_test.go | 1 + contrib/go-redis/redis.v8/example_test.go | 1 + contrib/go-redis/redis/example_test.go | 1 + contrib/gocql/gocql/example_test.go | 1 + contrib/gomodule/redigo/example_test.go | 1 + contrib/gomodule/redigo/redigo_test.go | 1 + contrib/google.golang.org/api/api_test.go | 1 + contrib/google.golang.org/api/example_test.go | 1 + contrib/google.golang.org/api/make_endpoints.go | 1 + .../google.golang.org/grpc.v12/fixtures_test.pb.go | 11 +++++++---- contrib/google.golang.org/grpc/fixtures_test.pb.go | 11 +++++++---- contrib/graph-gophers/graphql-go/example_test.go | 1 + contrib/graph-gophers/graphql-go/graphql_test.go | 1 + contrib/internal/httputil/trace_gen.go | 3 ++- contrib/k8s.io/client-go/kubernetes/example_test.go | 3 ++- contrib/miekg/dns/dns_test.go | 1 + contrib/miekg/dns/example_test.go | 1 + contrib/net/http/http_test.go | 1 + contrib/net/http/roundtripper_test.go | 1 + contrib/olivere/elastic/elastictrace_test.go | 5 +++-- contrib/olivere/elastic/example_test.go | 5 +++-- contrib/syndtr/goleveldb/leveldb/leveldb_test.go | 1 + contrib/tidwall/buntdb/buntdb_test.go | 1 + contrib/zenazn/goji.v1/web/example_test.go | 1 + ddtrace/tracer/stats.go | 2 +- profiler/options_test.go | 1 + 43 files changed, 64 insertions(+), 16 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 60b3876f4a..7f79646b05 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -63,7 +63,10 @@ jobs: - run: name: goimports command: | - if [ "$(goimports -e -l -local gopkg.in/DataDog/dd-trace-go.v1 . | wc -l)" -gt 0 ]; then + go install golang.org/x/tools/cmd/goimports + if [ "$(~/go/bin/goimports -e -l -local gopkg.in/DataDog/dd-trace-go.v1 . | wc -l)" -gt 0 ]; then + echo "Run 'goimports -w -local gopkg.in/DataDog/dd-trace-go.v1 .' to format code." + ~/go/bin/goimports -d -local gopkg.in/DataDog/dd-trace-go.v1 . exit 1 fi diff --git a/contrib/Shopify/sarama/example_test.go b/contrib/Shopify/sarama/example_test.go index ef1c218a02..64c083522f 100644 --- a/contrib/Shopify/sarama/example_test.go +++ b/contrib/Shopify/sarama/example_test.go @@ -9,6 +9,7 @@ import ( "log" "github.com/Shopify/sarama" + saramatrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/Shopify/sarama" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" ) diff --git a/contrib/aws/aws-sdk-go-v2/aws/example_test.go b/contrib/aws/aws-sdk-go-v2/aws/example_test.go index 9696b620e2..be6bde508e 100644 --- a/contrib/aws/aws-sdk-go-v2/aws/example_test.go +++ b/contrib/aws/aws-sdk-go-v2/aws/example_test.go @@ -11,6 +11,7 @@ import ( awscfg "github.com/aws/aws-sdk-go-v2/config" "github.com/aws/aws-sdk-go-v2/service/sqs" + awstrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/aws/aws-sdk-go-v2/aws" ) diff --git a/contrib/aws/aws-sdk-go/aws/aws_test.go b/contrib/aws/aws-sdk-go/aws/aws_test.go index a18a38b926..8a1906ea29 100644 --- a/contrib/aws/aws-sdk-go/aws/aws_test.go +++ b/contrib/aws/aws-sdk-go/aws/aws_test.go @@ -17,6 +17,7 @@ import ( "github.com/aws/aws-sdk-go/service/ec2" "github.com/aws/aws-sdk-go/service/s3" "github.com/stretchr/testify/assert" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" diff --git a/contrib/aws/aws-sdk-go/aws/example_test.go b/contrib/aws/aws-sdk-go/aws/example_test.go index f3a08ef9e5..584e0f02e1 100644 --- a/contrib/aws/aws-sdk-go/aws/example_test.go +++ b/contrib/aws/aws-sdk-go/aws/example_test.go @@ -9,6 +9,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/session" "github.com/aws/aws-sdk-go/service/s3" + awstrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/aws/aws-sdk-go/aws" ) diff --git a/contrib/bradfitz/gomemcache/memcache/example_test.go b/contrib/bradfitz/gomemcache/memcache/example_test.go index bb90cbedee..0ea7c32006 100644 --- a/contrib/bradfitz/gomemcache/memcache/example_test.go +++ b/contrib/bradfitz/gomemcache/memcache/example_test.go @@ -9,6 +9,7 @@ import ( "context" "github.com/bradfitz/gomemcache/memcache" + memcachetrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/bradfitz/gomemcache/memcache" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" ) diff --git a/contrib/bradfitz/gomemcache/memcache/memcache.go b/contrib/bradfitz/gomemcache/memcache/memcache.go index 2c5086cdb7..1c13f8660e 100644 --- a/contrib/bradfitz/gomemcache/memcache/memcache.go +++ b/contrib/bradfitz/gomemcache/memcache/memcache.go @@ -16,6 +16,7 @@ import ( "math" "github.com/bradfitz/gomemcache/memcache" + "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/tracer" diff --git a/contrib/bradfitz/gomemcache/memcache/memcache_test.go b/contrib/bradfitz/gomemcache/memcache/memcache_test.go index 745ceb69a1..cf23a663e3 100644 --- a/contrib/bradfitz/gomemcache/memcache/memcache_test.go +++ b/contrib/bradfitz/gomemcache/memcache/memcache_test.go @@ -16,6 +16,7 @@ import ( "github.com/bradfitz/gomemcache/memcache" "github.com/stretchr/testify/assert" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" diff --git a/contrib/confluentinc/confluent-kafka-go/kafka/example_test.go b/contrib/confluentinc/confluent-kafka-go/kafka/example_test.go index 87f6e6c5e4..f427b3e864 100644 --- a/contrib/confluentinc/confluent-kafka-go/kafka/example_test.go +++ b/contrib/confluentinc/confluent-kafka-go/kafka/example_test.go @@ -9,6 +9,7 @@ import ( "fmt" "github.com/confluentinc/confluent-kafka-go/kafka" + kafkatrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/confluentinc/confluent-kafka-go/kafka" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" ) diff --git a/contrib/confluentinc/confluent-kafka-go/kafka/option_test.go b/contrib/confluentinc/confluent-kafka-go/kafka/option_test.go index da446417c5..1ff74c80da 100644 --- a/contrib/confluentinc/confluent-kafka-go/kafka/option_test.go +++ b/contrib/confluentinc/confluent-kafka-go/kafka/option_test.go @@ -10,6 +10,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "gopkg.in/DataDog/dd-trace-go.v1/internal/globalconfig" ) diff --git a/contrib/database/sql/example_test.go b/contrib/database/sql/example_test.go index c139919684..7e94d6eec0 100644 --- a/contrib/database/sql/example_test.go +++ b/contrib/database/sql/example_test.go @@ -10,6 +10,7 @@ import ( "log" sqlite "github.com/mattn/go-sqlite3" // Setup application to use Sqlite + sqltrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/database/sql" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" diff --git a/contrib/database/sql/internal/dsn_test.go b/contrib/database/sql/internal/dsn_test.go index b043b1ed5f..ceba66317c 100644 --- a/contrib/database/sql/internal/dsn_test.go +++ b/contrib/database/sql/internal/dsn_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" ) diff --git a/contrib/emicklei/go-restful/example_test.go b/contrib/emicklei/go-restful/example_test.go index 2ea8c38cbe..546e4a63b8 100644 --- a/contrib/emicklei/go-restful/example_test.go +++ b/contrib/emicklei/go-restful/example_test.go @@ -11,6 +11,7 @@ import ( "net/http" "github.com/emicklei/go-restful" + restfultrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/emicklei/go-restful" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" ) diff --git a/contrib/emicklei/go-restful/restful_test.go b/contrib/emicklei/go-restful/restful_test.go index ae9dc60d6a..eeaece6bed 100644 --- a/contrib/emicklei/go-restful/restful_test.go +++ b/contrib/emicklei/go-restful/restful_test.go @@ -13,6 +13,7 @@ import ( "github.com/emicklei/go-restful" "github.com/stretchr/testify/assert" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" diff --git a/contrib/gin-gonic/gin/option.go b/contrib/gin-gonic/gin/option.go index 59d03f753b..7271f8e2dd 100644 --- a/contrib/gin-gonic/gin/option.go +++ b/contrib/gin-gonic/gin/option.go @@ -10,6 +10,7 @@ import ( "net/http" "github.com/gin-gonic/gin" + "gopkg.in/DataDog/dd-trace-go.v1/internal" "gopkg.in/DataDog/dd-trace-go.v1/internal/globalconfig" ) diff --git a/contrib/globalsign/mgo/mgo_test.go b/contrib/globalsign/mgo/mgo_test.go index 80f9712825..f235e90805 100644 --- a/contrib/globalsign/mgo/mgo_test.go +++ b/contrib/globalsign/mgo/mgo_test.go @@ -14,6 +14,7 @@ import ( "github.com/globalsign/mgo" "github.com/globalsign/mgo/bson" "github.com/stretchr/testify/assert" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" diff --git a/contrib/globalsign/mgo/pipe.go b/contrib/globalsign/mgo/pipe.go index 6d5f562964..1d4f179be3 100644 --- a/contrib/globalsign/mgo/pipe.go +++ b/contrib/globalsign/mgo/pipe.go @@ -7,6 +7,7 @@ package mgo import ( "github.com/globalsign/mgo" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" ) diff --git a/contrib/go-redis/redis.v7/example_test.go b/contrib/go-redis/redis.v7/example_test.go index 455d157d76..ed16ef79b5 100644 --- a/contrib/go-redis/redis.v7/example_test.go +++ b/contrib/go-redis/redis.v7/example_test.go @@ -10,6 +10,7 @@ import ( "time" "github.com/go-redis/redis/v7" + redistrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/go-redis/redis.v7" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" diff --git a/contrib/go-redis/redis.v8/example_test.go b/contrib/go-redis/redis.v8/example_test.go index f5287259e5..20db897ae7 100644 --- a/contrib/go-redis/redis.v8/example_test.go +++ b/contrib/go-redis/redis.v8/example_test.go @@ -10,6 +10,7 @@ import ( "time" "github.com/go-redis/redis/v8" + redistrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/go-redis/redis.v8" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" diff --git a/contrib/go-redis/redis/example_test.go b/contrib/go-redis/redis/example_test.go index 0145c50e5d..ffb47b01ef 100644 --- a/contrib/go-redis/redis/example_test.go +++ b/contrib/go-redis/redis/example_test.go @@ -10,6 +10,7 @@ import ( "time" "github.com/go-redis/redis" + redistrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/go-redis/redis" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" diff --git a/contrib/gocql/gocql/example_test.go b/contrib/gocql/gocql/example_test.go index 5c307d5584..2d16387391 100644 --- a/contrib/gocql/gocql/example_test.go +++ b/contrib/gocql/gocql/example_test.go @@ -9,6 +9,7 @@ import ( "context" "github.com/gocql/gocql" + gocqltrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/gocql/gocql" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" diff --git a/contrib/gomodule/redigo/example_test.go b/contrib/gomodule/redigo/example_test.go index 0c11bc30a3..1b5ab00c49 100644 --- a/contrib/gomodule/redigo/example_test.go +++ b/contrib/gomodule/redigo/example_test.go @@ -11,6 +11,7 @@ import ( "time" "github.com/gomodule/redigo/redis" + redigotrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/gomodule/redigo" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" ) diff --git a/contrib/gomodule/redigo/redigo_test.go b/contrib/gomodule/redigo/redigo_test.go index 13167a396a..237409f01c 100644 --- a/contrib/gomodule/redigo/redigo_test.go +++ b/contrib/gomodule/redigo/redigo_test.go @@ -14,6 +14,7 @@ import ( "github.com/gomodule/redigo/redis" "github.com/stretchr/testify/assert" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" diff --git a/contrib/google.golang.org/api/api_test.go b/contrib/google.golang.org/api/api_test.go index c90a2ac47a..b8f602fdb7 100644 --- a/contrib/google.golang.org/api/api_test.go +++ b/contrib/google.golang.org/api/api_test.go @@ -15,6 +15,7 @@ import ( books "google.golang.org/api/books/v1" civicinfo "google.golang.org/api/civicinfo/v2" urlshortener "google.golang.org/api/urlshortener/v1" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer" "gopkg.in/DataDog/dd-trace-go.v1/internal/globalconfig" diff --git a/contrib/google.golang.org/api/example_test.go b/contrib/google.golang.org/api/example_test.go index eaea0b8f49..0ba90c7205 100644 --- a/contrib/google.golang.org/api/example_test.go +++ b/contrib/google.golang.org/api/example_test.go @@ -9,6 +9,7 @@ import ( "fmt" cloudresourcemanager "google.golang.org/api/cloudresourcemanager/v1" + apitrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/google.golang.org/api" ) diff --git a/contrib/google.golang.org/api/make_endpoints.go b/contrib/google.golang.org/api/make_endpoints.go index 284b2e771b..b73ebeef01 100644 --- a/contrib/google.golang.org/api/make_endpoints.go +++ b/contrib/google.golang.org/api/make_endpoints.go @@ -21,6 +21,7 @@ import ( "text/template" "github.com/yosida95/uritemplate" + "gopkg.in/DataDog/dd-trace-go.v1/contrib/google.golang.org/api/internal" ) diff --git a/contrib/google.golang.org/grpc.v12/fixtures_test.pb.go b/contrib/google.golang.org/grpc.v12/fixtures_test.pb.go index 28f1bba616..c1f2c99fcb 100644 --- a/contrib/google.golang.org/grpc.v12/fixtures_test.pb.go +++ b/contrib/google.golang.org/grpc.v12/fixtures_test.pb.go @@ -18,12 +18,15 @@ It has these top-level messages: */ package grpc -import proto "github.com/golang/protobuf/proto" -import fmt "fmt" -import math "math" - import ( + fmt "fmt" + + proto "github.com/golang/protobuf/proto" + + math "math" + context "golang.org/x/net/context" + grpc1 "google.golang.org/grpc" ) diff --git a/contrib/google.golang.org/grpc/fixtures_test.pb.go b/contrib/google.golang.org/grpc/fixtures_test.pb.go index 894f76bc30..823b62cb7b 100644 --- a/contrib/google.golang.org/grpc/fixtures_test.pb.go +++ b/contrib/google.golang.org/grpc/fixtures_test.pb.go @@ -18,12 +18,15 @@ It has these top-level messages: */ package grpc -import proto "github.com/golang/protobuf/proto" -import fmt "fmt" -import math "math" - import ( + fmt "fmt" + + proto "github.com/golang/protobuf/proto" + + math "math" + context "golang.org/x/net/context" + grpc1 "google.golang.org/grpc" ) diff --git a/contrib/graph-gophers/graphql-go/example_test.go b/contrib/graph-gophers/graphql-go/example_test.go index 6b4fda9677..c99bdeb626 100644 --- a/contrib/graph-gophers/graphql-go/example_test.go +++ b/contrib/graph-gophers/graphql-go/example_test.go @@ -11,6 +11,7 @@ import ( graphql "github.com/graph-gophers/graphql-go" "github.com/graph-gophers/graphql-go/relay" + graphqltrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/graph-gophers/graphql-go" ) diff --git a/contrib/graph-gophers/graphql-go/graphql_test.go b/contrib/graph-gophers/graphql-go/graphql_test.go index 6e48ae4a25..6b3d5b4676 100644 --- a/contrib/graph-gophers/graphql-go/graphql_test.go +++ b/contrib/graph-gophers/graphql-go/graphql_test.go @@ -14,6 +14,7 @@ import ( graphql "github.com/graph-gophers/graphql-go" "github.com/graph-gophers/graphql-go/relay" "github.com/stretchr/testify/assert" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer" "gopkg.in/DataDog/dd-trace-go.v1/internal/globalconfig" diff --git a/contrib/internal/httputil/trace_gen.go b/contrib/internal/httputil/trace_gen.go index 30c2567261..79eda8aaae 100644 --- a/contrib/internal/httputil/trace_gen.go +++ b/contrib/internal/httputil/trace_gen.go @@ -8,8 +8,9 @@ package httputil import ( - "gopkg.in/DataDog/dd-trace-go.v1/ddtrace" "net/http" + + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace" ) // wrapResponseWriter wraps an underlying http.ResponseWriter so that it can diff --git a/contrib/k8s.io/client-go/kubernetes/example_test.go b/contrib/k8s.io/client-go/kubernetes/example_test.go index a7cf565565..eb57074efd 100644 --- a/contrib/k8s.io/client-go/kubernetes/example_test.go +++ b/contrib/k8s.io/client-go/kubernetes/example_test.go @@ -8,11 +8,12 @@ package kubernetes_test import ( "fmt" - kubernetestrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/k8s.io/client-go/kubernetes" meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" _ "k8s.io/client-go/plugin/pkg/client/auth/oidc" "k8s.io/client-go/rest" + + kubernetestrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/k8s.io/client-go/kubernetes" ) func Example() { diff --git a/contrib/miekg/dns/dns_test.go b/contrib/miekg/dns/dns_test.go index 789b78d258..94c78d261a 100644 --- a/contrib/miekg/dns/dns_test.go +++ b/contrib/miekg/dns/dns_test.go @@ -12,6 +12,7 @@ import ( "github.com/miekg/dns" "github.com/stretchr/testify/assert" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer" ) diff --git a/contrib/miekg/dns/example_test.go b/contrib/miekg/dns/example_test.go index bed18632a9..f54349d0ba 100644 --- a/contrib/miekg/dns/example_test.go +++ b/contrib/miekg/dns/example_test.go @@ -9,6 +9,7 @@ import ( "fmt" "github.com/miekg/dns" + dnstrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/miekg/dns" ) diff --git a/contrib/net/http/http_test.go b/contrib/net/http/http_test.go index 669c060253..9cdf5b511b 100644 --- a/contrib/net/http/http_test.go +++ b/contrib/net/http/http_test.go @@ -11,6 +11,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" diff --git a/contrib/net/http/roundtripper_test.go b/contrib/net/http/roundtripper_test.go index 5118f24c2d..f31da1466d 100644 --- a/contrib/net/http/roundtripper_test.go +++ b/contrib/net/http/roundtripper_test.go @@ -13,6 +13,7 @@ import ( "time" "github.com/stretchr/testify/assert" + "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/mocktracer" diff --git a/contrib/olivere/elastic/elastictrace_test.go b/contrib/olivere/elastic/elastictrace_test.go index 2d91ddd8c4..b79957ea4c 100644 --- a/contrib/olivere/elastic/elastictrace_test.go +++ b/contrib/olivere/elastic/elastictrace_test.go @@ -15,11 +15,12 @@ import ( "os" "github.com/stretchr/testify/assert" + elasticv3 "gopkg.in/olivere/elastic.v3" + elasticv5 "gopkg.in/olivere/elastic.v5" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer" "gopkg.in/DataDog/dd-trace-go.v1/internal/globalconfig" - elasticv3 "gopkg.in/olivere/elastic.v3" - elasticv5 "gopkg.in/olivere/elastic.v5" "testing" ) diff --git a/contrib/olivere/elastic/example_test.go b/contrib/olivere/elastic/example_test.go index dfc1c04fae..5789083064 100644 --- a/contrib/olivere/elastic/example_test.go +++ b/contrib/olivere/elastic/example_test.go @@ -8,10 +8,11 @@ package elastic_test import ( "context" - elastictrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/olivere/elastic" - "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" elasticv3 "gopkg.in/olivere/elastic.v3" elasticv5 "gopkg.in/olivere/elastic.v5" + + elastictrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/olivere/elastic" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" ) // To start tracing elastic.v5 requests, create a new TracedHTTPClient that you will diff --git a/contrib/syndtr/goleveldb/leveldb/leveldb_test.go b/contrib/syndtr/goleveldb/leveldb/leveldb_test.go index 4ceabd590c..9af824b25f 100644 --- a/contrib/syndtr/goleveldb/leveldb/leveldb_test.go +++ b/contrib/syndtr/goleveldb/leveldb/leveldb_test.go @@ -14,6 +14,7 @@ import ( "github.com/syndtr/goleveldb/leveldb/opt" "github.com/syndtr/goleveldb/leveldb/storage" "github.com/syndtr/goleveldb/leveldb/util" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" diff --git a/contrib/tidwall/buntdb/buntdb_test.go b/contrib/tidwall/buntdb/buntdb_test.go index 9807435422..940ac08ca4 100644 --- a/contrib/tidwall/buntdb/buntdb_test.go +++ b/contrib/tidwall/buntdb/buntdb_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/tidwall/buntdb" + "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" diff --git a/contrib/zenazn/goji.v1/web/example_test.go b/contrib/zenazn/goji.v1/web/example_test.go index 32e5a33046..af29e1dd3b 100644 --- a/contrib/zenazn/goji.v1/web/example_test.go +++ b/contrib/zenazn/goji.v1/web/example_test.go @@ -11,6 +11,7 @@ import ( "github.com/zenazn/goji" "github.com/zenazn/goji/web" + webtrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/zenazn/goji.v1/web" ) diff --git a/ddtrace/tracer/stats.go b/ddtrace/tracer/stats.go index ad62f2c155..f6944ecd1d 100644 --- a/ddtrace/tracer/stats.go +++ b/ddtrace/tracer/stats.go @@ -12,11 +12,11 @@ import ( "sync/atomic" "time" - "google.golang.org/protobuf/proto" "gopkg.in/DataDog/dd-trace-go.v1/internal/log" "github.com/DataDog/datadog-go/statsd" "github.com/DataDog/sketches-go/ddsketch" + "google.golang.org/protobuf/proto" ) // aggregableSpan holds necessary information about a span that can be used to diff --git a/profiler/options_test.go b/profiler/options_test.go index e6389a5987..f782c652c1 100644 --- a/profiler/options_test.go +++ b/profiler/options_test.go @@ -17,6 +17,7 @@ import ( "github.com/DataDog/datadog-go/statsd" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gopkg.in/DataDog/dd-trace-go.v1/internal/globalconfig" ) From d326bfb295a7c76347e5866bb349b9fb8e6fe7b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Geisend=C3=B6rfer?= Date: Wed, 18 Aug 2021 00:28:52 +0200 Subject: [PATCH 5/8] profiler: Implement Delta Profiles (#842) This patch implements delta profiles which will allow us to enable allocation, mutex and block profiles in the aggregation and comparison views of our web ui. The internal google doc "RFC: Go Profiling: Delta Profiles" describes this in great detail. To simplify the code, a refactoring was done that attempts to increase code sharing between similar profile types, in particular heap, mutex, block and goroutine profiles (see type profileType). Additionally the new profiler.enabledProfileTypes() method ensures that profiles are collected in a deterministic order. Testing is accomplished by the new pprofutils package which allows converting profiles between protobuf and a simplified text format. Since that package also contains a suitable delta profiling implementation, it's used for the delta profiling itself as well. In this iteration, delta profiles are uploaded in addition to the original profiles using a "delta-" prefix, e.g. "delta-mutex.pprof". This is done to avoid breaking things until the backend has made corresponding changes as well. The plan for the next iteration is to stop uploading the original profiles since they are redundant and a waste of bandwidth and storage. One particular complexity worth noting is that the "delta-heap.pprof" contains 2 profiles alloc_ and inuse_. Only the alloc_ sample types are subject to delta computation, the inuse_ ones are kept as-is since they describe the current state of the heap's live set. --- go.mod | 2 +- go.sum | 3 + profiler/internal/pprofutils/README.md | 7 + profiler/internal/pprofutils/delta.go | 66 ++++ profiler/internal/pprofutils/delta_test.go | 85 ++++ profiler/internal/pprofutils/pprofutils.go | 13 + profiler/internal/pprofutils/protobuf.go | 67 ++++ profiler/internal/pprofutils/protobuf_test.go | 59 +++ .../test-fixtures/pprof.lines.pb.gz | Bin 0 -> 940 bytes .../test-fixtures/pprof.samples.cpu.001.pb.gz | Bin 0 -> 1298 bytes profiler/internal/pprofutils/text.go | 118 ++++++ profiler/internal/pprofutils/text_test.go | 57 +++ profiler/profile.go | 368 ++++++++++-------- profiler/profile_test.go | 252 +++++++++--- profiler/profiler.go | 56 ++- profiler/profiler_test.go | 19 +- 16 files changed, 938 insertions(+), 234 deletions(-) create mode 100644 profiler/internal/pprofutils/README.md create mode 100644 profiler/internal/pprofutils/delta.go create mode 100644 profiler/internal/pprofutils/delta_test.go create mode 100644 profiler/internal/pprofutils/pprofutils.go create mode 100644 profiler/internal/pprofutils/protobuf.go create mode 100644 profiler/internal/pprofutils/protobuf_test.go create mode 100644 profiler/internal/pprofutils/test-fixtures/pprof.lines.pb.gz create mode 100644 profiler/internal/pprofutils/test-fixtures/pprof.samples.cpu.001.pb.gz create mode 100644 profiler/internal/pprofutils/text.go create mode 100644 profiler/internal/pprofutils/text_test.go diff --git a/go.mod b/go.mod index ed0aa6f643..08aac6dbb8 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,6 @@ require ( github.com/DataDog/datadog-go v4.4.0+incompatible github.com/DataDog/gostackparse v0.5.0 github.com/DataDog/sketches-go v1.0.0 - github.com/google/pprof v0.0.0-20210125172800-10e9aeb4a998 + github.com/google/pprof v0.0.0-20210423192551-a2663126120b github.com/tinylib/msgp v1.1.2 ) diff --git a/go.sum b/go.sum index a073f41034..b02926739f 100644 --- a/go.sum +++ b/go.sum @@ -259,6 +259,8 @@ github.com/google/pprof v0.0.0-20200229191704-1ebb73c60ed3/go.mod h1:ZgVRPoUq/hf github.com/google/pprof v0.0.0-20200430221834-fc25d7d30c6d/go.mod h1:ZgVRPoUq/hfqzAqh7sHMqb3I9Rq5C59dIz2SbBwJ4eM= github.com/google/pprof v0.0.0-20200708004538-1a94d8640e99/go.mod h1:ZgVRPoUq/hfqzAqh7sHMqb3I9Rq5C59dIz2SbBwJ4eM= github.com/google/pprof v0.0.0-20210125172800-10e9aeb4a998/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE= +github.com/google/pprof v0.0.0-20210423192551-a2663126120b h1:l2YRhr+YLzmSp7KJMswRVk/lO5SwoFIcCLzJsVj+YPc= +github.com/google/pprof v0.0.0-20210423192551-a2663126120b/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE= github.com/google/renameio v0.1.0/go.mod h1:KWCgfxg9yswjAJkECMjeO8J8rahYeXnNhOm40UhjYkI= github.com/google/uuid v1.1.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/google/uuid v1.1.2 h1:EVhdT+1Kseyi1/pUmXKaFxYsDNy9RQYkMWRH68J/W7Y= @@ -539,6 +541,7 @@ github.com/stretchr/testify v1.6.0 h1:jlIyCplCJFULU/01vCkhKuTyc3OorI3bJFuw6obfgh github.com/stretchr/testify v1.6.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.6.1 h1:hDPOHmpOpP40lSULcqw7IrRb/u7w6RpDC9399XyoNd0= github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY= github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/syndtr/goleveldb v1.0.0 h1:fBdIW9lB4Iz0n9khmH8w27SJ3QEJ7+IgjPEwGSZiFdE= github.com/syndtr/goleveldb v1.0.0/go.mod h1:ZVVdQEZoIme9iO1Ch2Jdy24qqXrMMOU6lpPAyBWyWuQ= diff --git a/profiler/internal/pprofutils/README.md b/profiler/internal/pprofutils/README.md new file mode 100644 index 0000000000..cf3eb11826 --- /dev/null +++ b/profiler/internal/pprofutils/README.md @@ -0,0 +1,7 @@ +# pprofutils + +Internal fork of https://github.com/felixge/pprofutils stripped to only include +essential code and tests. It's used for delta profiles as well as testing. + +It'd be nice to keep this in sync with upstream, but no worries if not. We just +need the delta profile stuff to work. diff --git a/profiler/internal/pprofutils/delta.go b/profiler/internal/pprofutils/delta.go new file mode 100644 index 0000000000..1326db36b7 --- /dev/null +++ b/profiler/internal/pprofutils/delta.go @@ -0,0 +1,66 @@ +// 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 2021 Datadog, Inc. + +package pprofutils + +import ( + "errors" + + "github.com/google/pprof/profile" +) + +// Delta describes how to compute the delta between two profiles and implements +// the conversion. +type Delta struct { + // SampleTypes limits the delta calcultion to the given sample types. Other + // sample types will retain the values of profile b. The defined sample types + // must exist in the profile, otherwise derivation will fail with an error. + // If the slice is empty, all sample types are subject to delta profile + // derivation. + // + // The use case for this for this is to deal with the heap profile which + // contains alloc and inuse sample types, but delta profiling makes no sense + // for the latter. + SampleTypes []ValueType +} + +// Convert computes the delta between all values b-a and returns them as a new +// profile. Samples that end up with a delta of 0 are dropped. WARNING: Profile +// a will be mutated by this function. You should pass a copy if that's +// undesirable. +func (d Delta) Convert(a, b *profile.Profile) (*profile.Profile, error) { + ratios := make([]float64, len(a.SampleType)) + + found := 0 + for i, st := range a.SampleType { + // Empty c.SampleTypes means we calculate the delta for every st + if len(d.SampleTypes) == 0 { + ratios[i] = -1 + continue + } + + // Otherwise we only calcuate the delta for any st that is listed in + // c.SampleTypes. st's not listed in there will default to ratio 0, which + // means we delete them from pa, so only the pb values remain in the final + // profile. + for _, deltaSt := range d.SampleTypes { + if deltaSt.Type == st.Type && deltaSt.Unit == st.Unit { + ratios[i] = -1 + found++ + } + } + } + if found != len(d.SampleTypes) { + return nil, errors.New("one or more sample type(s) was not found in the profile") + } + + a.ScaleN(ratios) + + delta, err := profile.Merge([]*profile.Profile{a, b}) + if err != nil { + return nil, err + } + return delta, delta.CheckValid() +} diff --git a/profiler/internal/pprofutils/delta_test.go b/profiler/internal/pprofutils/delta_test.go new file mode 100644 index 0000000000..c922aa2847 --- /dev/null +++ b/profiler/internal/pprofutils/delta_test.go @@ -0,0 +1,85 @@ +// 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 2021 Datadog, Inc. + +package pprofutils + +import ( + "bytes" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestDelta(t *testing.T) { + t.Run("simple", func(t *testing.T) { + var deltaText bytes.Buffer + + profA, err := Text{}.Convert(strings.NewReader(strings.TrimSpace(` +main;foo 5 +main;foo;bar 3 +main;foobar 4 +`))) + require.NoError(t, err) + + profB, err := Text{}.Convert(strings.NewReader(strings.TrimSpace(` +main;foo 8 +main;foo;bar 3 +main;foobar 5 +`))) + require.NoError(t, err) + + delta, err := Delta{}.Convert(profA, profB) + require.NoError(t, err) + + require.NoError(t, Protobuf{}.Convert(delta, &deltaText)) + require.Equal(t, deltaText.String(), strings.TrimSpace(` +main;foo 3 +main;foobar 1 +`)+"\n") + }) + + t.Run("sampleTypes", func(t *testing.T) { + profA, err := Text{}.Convert(strings.NewReader(strings.TrimSpace(` +x/count y/count +main;foo 5 10 +main;foo;bar 3 6 +main;foo;baz 9 0 +main;foobar 4 8 +`))) + require.NoError(t, err) + + profB, err := Text{}.Convert(strings.NewReader(strings.TrimSpace(` +x/count y/count +main;foo 8 16 +main;foo;bar 3 6 +main;foo;baz 9 0 +main;foobar 5 10 +`))) + require.NoError(t, err) + + t.Run("happyPath", func(t *testing.T) { + var deltaText bytes.Buffer + + deltaConfig := Delta{SampleTypes: []ValueType{{Type: "x", Unit: "count"}}} + delta, err := deltaConfig.Convert(profA, profB) + require.NoError(t, err) + + require.NoError(t, Protobuf{SampleTypes: true}.Convert(delta, &deltaText)) + require.Equal(t, deltaText.String(), strings.TrimSpace(` +x/count y/count +main;foo 3 16 +main;foobar 1 10 +main;foo;bar 0 6 +`)+"\n") + }) + + t.Run("unknownSampleType", func(t *testing.T) { + deltaConfig := Delta{SampleTypes: []ValueType{{Type: "foo", Unit: "count"}}} + _, err := deltaConfig.Convert(profA, profB) + require.Equal(t, "one or more sample type(s) was not found in the profile", err.Error()) + }) + }) +} diff --git a/profiler/internal/pprofutils/pprofutils.go b/profiler/internal/pprofutils/pprofutils.go new file mode 100644 index 0000000000..114c014eef --- /dev/null +++ b/profiler/internal/pprofutils/pprofutils.go @@ -0,0 +1,13 @@ +// 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 2021 Datadog, Inc. + +// Package pprofutils is a fork of github.com/felixge/pprofutils, see README. +package pprofutils + +// ValueType describes the type and unit of a value. +type ValueType struct { + Type string + Unit string +} diff --git a/profiler/internal/pprofutils/protobuf.go b/profiler/internal/pprofutils/protobuf.go new file mode 100644 index 0000000000..9aed70915b --- /dev/null +++ b/profiler/internal/pprofutils/protobuf.go @@ -0,0 +1,67 @@ +// 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 2021 Datadog, Inc. + +package pprofutils + +import ( + "bufio" + "fmt" + "io" + "sort" + "strings" + + "github.com/google/pprof/profile" +) + +// Protobuf converts from pprof's protobuf to folded text format. +type Protobuf struct { + // SampleTypes causes the text output to begin with a header line listing + // the sample types found in the profile. This is a custom extension to the + // folded text format. + SampleTypes bool +} + +// Convert marshals the given protobuf profile into folded text format. +func (p Protobuf) Convert(protobuf *profile.Profile, text io.Writer) error { + w := bufio.NewWriter(text) + if p.SampleTypes { + var sampleTypes []string + for _, sampleType := range protobuf.SampleType { + sampleTypes = append(sampleTypes, sampleType.Type+"/"+sampleType.Unit) + } + w.WriteString(strings.Join(sampleTypes, " ") + "\n") + } + if err := protobuf.Aggregate(true, true, false, false, false); err != nil { + return err + } + protobuf = protobuf.Compact() + sort.Slice(protobuf.Sample, func(i, j int) bool { + return protobuf.Sample[i].Value[0] > protobuf.Sample[j].Value[0] + }) + for _, sample := range protobuf.Sample { + var frames []string + for i := range sample.Location { + loc := sample.Location[len(sample.Location)-i-1] + for j := range loc.Line { + line := loc.Line[len(loc.Line)-j-1] + frames = append(frames, line.Function.Name) + } + } + var values []string + for _, val := range sample.Value { + values = append(values, fmt.Sprintf("%d", val)) + if !p.SampleTypes { + break + } + } + fmt.Fprintf( + w, + "%s %s\n", + strings.Join(frames, ";"), + strings.Join(values, " "), + ) + } + return w.Flush() +} diff --git a/profiler/internal/pprofutils/protobuf_test.go b/profiler/internal/pprofutils/protobuf_test.go new file mode 100644 index 0000000000..5cac0f240c --- /dev/null +++ b/profiler/internal/pprofutils/protobuf_test.go @@ -0,0 +1,59 @@ +// 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 2021 Datadog, Inc. + +package pprofutils + +import ( + "bytes" + "io/ioutil" + "path/filepath" + "strings" + "testing" + + "github.com/google/pprof/profile" + "github.com/stretchr/testify/require" +) + +func TestProtobufConvert(t *testing.T) { + t.Run("basic", func(t *testing.T) { + data, err := ioutil.ReadFile(filepath.Join("test-fixtures", "pprof.samples.cpu.001.pb.gz")) + require.NoError(t, err) + + proto, err := profile.Parse(bytes.NewReader(data)) + require.NoError(t, err) + + out := bytes.Buffer{} + require.NoError(t, Protobuf{}.Convert(proto, &out)) + want := strings.TrimSpace(` +golang.org/x/sync/errgroup.(*Group).Go.func1;main.run.func2;main.computeSum 19 +runtime.mcall;runtime.park_m;runtime.resetForSleep;runtime.resettimer;runtime.modtimer;runtime.wakeNetPoller;runtime.netpollBreak;runtime.write;runtime.write1 7 +golang.org/x/sync/errgroup.(*Group).Go.func1;main.run.func2;main.computeSum;runtime.asyncPreempt 5 +runtime.mstart;runtime.mstart1;runtime.sysmon;runtime.usleep 3 +runtime.mcall;runtime.park_m;runtime.schedule;runtime.findrunnable;runtime.stopm;runtime.notesleep;runtime.semasleep;runtime.pthread_cond_wait 2 +runtime.mcall;runtime.gopreempt_m;runtime.goschedImpl;runtime.schedule;runtime.findrunnable;runtime.stopm;runtime.notesleep;runtime.semasleep;runtime.pthread_cond_wait 1 +runtime.mcall;runtime.park_m;runtime.schedule;runtime.findrunnable;runtime.checkTimers;runtime.nanotime;runtime.nanotime1 1 +`) + "\n" + require.Equal(t, out.String(), want) + }) + + t.Run("differentLinesPerFunction", func(t *testing.T) { + data, err := ioutil.ReadFile(filepath.Join("test-fixtures", "pprof.lines.pb.gz")) + require.NoError(t, err) + + proto, err := profile.Parse(bytes.NewReader(data)) + require.NoError(t, err) + + out := bytes.Buffer{} + require.NoError(t, Protobuf{}.Convert(proto, &out)) + want := strings.TrimSpace(` +main.run.func1;main.threadKind.Run;main.goGo1;main.goHog 85 +main.run.func1;main.threadKind.Run;main.goGo2;main.goHog 78 +main.run.func1;main.threadKind.Run;main.goGo3;main.goHog 72 +main.run.func1;main.threadKind.Run;main.goGo0;main.goHog 72 +main.run.func1;main.threadKind.Run;main.goGo0;main.goHog;runtime.asyncPreempt 1 +`) + "\n" + require.Equal(t, out.String(), want) + }) +} diff --git a/profiler/internal/pprofutils/test-fixtures/pprof.lines.pb.gz b/profiler/internal/pprofutils/test-fixtures/pprof.lines.pb.gz new file mode 100644 index 0000000000000000000000000000000000000000..1b4edefa7b92c0e8ff46fd8c7f597db833dab197 GIT binary patch literal 940 zcmV;d15^ATiwFP!00004|9p{4h~3m5$N#xE_ueyUXXgB;(|RVGUM`|nadMKA$0QrK z9T21l-B`-Zc_oe9B$qtOba%im1Q&Hv7lJT573{_->axiASPP;k%Ebrbq6<;%ViuKT zS{xTPo9~a`_w#$4U$zfF|K#Hj{`%&NbA}Ey*fVs{hx)}ke}4Lg{&E-Zs@p&669_c) z^Y;h6$pC0jX;6C>01Xb%U_6|xf*v~j_MkV>K@YwHTdN00Xw+GKbnmN!*6PFEuBs0K z0-dF!`#*G+4xjHV9WJ4z@o;SgUV`fPcW(6!UF-Gr$toD28{c*40A?LJfCe%;^hKyX zy#Ghf(6vE_4$gcmCTXfi=BXr|Q3fJ8%EB zR@=fe_PS{T9jd#pw<)9+H(MR6g*H8FpEEbW04jB>%^|gTyVbE8PoJ=nxekUPGmhM@JOe>j-LYA>CaZ=!_RA6(LBLPfvb|^ zvdQZ-lXyNaix}6kss*hnW+|;|S=BhBX>KJ2F*2IA^DYz)Ig_=s>#L=t;)OI9)=N!J zR=Q-_ns*e2^`leg0?T3yfi#Zt;_UXy2VQQu zXeI*Bs>l@A*&HXuCTr4}@cqDdNaT8$*|vk_p2Y0oJIIs>B0@=G&*qLRs6#^G5h;a; zV~08-id{kl_XF}2?#8DK?}fom;BC&*yxH4K@&?aR#y5%OI#yMXsU1ZuBE)B$*}`@r z5sAPKLyt*6bR}~g#{AgxBzYP?u4BUaPGwbv<&ddInQ&Z}^U#*I!Gz!n&=l|HpRS(E55wgD6(5P{ z#*m;3anTmlcX~QqRky5%47)KBS*SdM3W>zT5Q7U;FrWbqaREEyLK783j2Jh@6>cP& zfVsD4x+NNuaZ~l(FXw#cJLlZRO~rsQ=% zT=@9h8Yw(FHa>w-TsU+lGZq7~gJ@YGNa)?~ZX`8nX`_~f2on0sO+`d-wU&h_5_Wg&)yetP;8XfcRkLd!xN3B9wVv0g(MLPAfz^g3v9h~t}4Eei=G^z7+ZWX=%o z)DQp(eeshQWJm%p5)C1Vg#LKrh-3f{W;KKq5_;w3qmoJD#e#;AMnXS)TQMp8y{I7! zBcazXC?<^`lr@AAB=pNmiW$bwC6hryFJE~>G9!2;q9J6F&|mMqCYcPLifCm(B=q2W zN2S3m7PL4ZUMpz`IVAL_-;VS=+{$SPc_j44Ii;V&(^9{Hgnn~Jspau-L@Pm2_HshC zTfkLX42t-ahEPI6KRT&WOL$#2RF?8*)TGMzeN?MJ1qr=+STPkm7SrND@tT|uMM9rH zzgYLAtUHQ?9_`f~#Y3{MF*)W-$0Rd`7uIPA<4EY4^UD7?zM9k!CXmo;?<-~kZ$S-! zV4TLQLp6X1CTK#TC<29QvIY>t6irprHGnt{)8Xn!4PXc}G*ivi01}84tGOBgV4miy zg&IH-i?pav3QM%4P#Vj$tk5u4XhoqBq?A@iYXBJ>qhm@ri{o^>I#B~aTuE1|xE!vc zs}#!PYPwp*6|hRHDz1oY=$fil11RBIx>m)NaUES(U0(yJ;0C%uA&ML6MukRk6Wyd# z#&9#;tTM*2Mr#U9;1;^2T3{4HQkV<9m5emH-*#kS=Kf>PTdr? z8wPw&XtSsFCw=bu`Yg9P^Yu3G_}pm;?#*&fZwuY`OugL+=E8k4sTbAedRt7oo|x@e z+?#Ynz2OLx9p4aw7=+-yY zcFF(d`YxfOrYi_r#+oGlaMe~8@8udoKG07UNF~u6wY%ANw8k3Eg=^fMN%*<9} ztI;nzsNWW>=!H(uv3cE)GP^y_Z8u;$_0acpOPGeGKg=!5P}QgEQ`_p>b*r-DjgqvTiWvaii6gt7z^wIsvL4KN!m^Hf6 z`JfAPj+@;lIyU!wmg(Ou!|OKf!Fj*e3Ha2YOLZQM(cytBEbAeU8(sF`a-A(^H@nY< zJXGdN{W?aMKhA^Q!m_wGn1U@@^6xGG1}rX)<(H&9?(?80R=S6KM0k7DmF*89^zS|U z2Yg#NEZcYG2Zk3+4FY<^qwkz}>x;`5j-M^>zH|KcSLwa#-Sdomy0|O;2mk>8|Fz@1 IkP8U_01TgehyVZp literal 0 HcmV?d00001 diff --git a/profiler/internal/pprofutils/text.go b/profiler/internal/pprofutils/text.go new file mode 100644 index 0000000000..acdc8b84d2 --- /dev/null +++ b/profiler/internal/pprofutils/text.go @@ -0,0 +1,118 @@ +// 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 2021 Datadog, Inc. + +package pprofutils + +import ( + "fmt" + "io" + "io/ioutil" + "strconv" + "strings" + "time" + + "github.com/google/pprof/profile" +) + +// Text converts from folded text to protobuf format. +type Text struct{} + +// Convert parses the given text and returns it as protobuf profile. +func (c Text) Convert(text io.Reader) (*profile.Profile, error) { + var ( + functionID = uint64(1) + locationID = uint64(1) + p = &profile.Profile{ + TimeNanos: time.Now().UnixNano(), + SampleType: []*profile.ValueType{{ + Type: "samples", + Unit: "count", + }}, + // Without his, Delta.Convert() fails in profile.Merge(). Perhaps an + // issue that's worth reporting upstream. + PeriodType: &profile.ValueType{}, + } + ) + + m := &profile.Mapping{ID: 1, HasFunctions: true} + p.Mapping = []*profile.Mapping{m} + + lines, err := ioutil.ReadAll(text) + if err != nil { + return nil, err + } + for n, line := range strings.Split(string(lines), "\n") { + if strings.TrimSpace(line) == "" { + continue + } + + // custom extension: first line can contain header that looks like this: + // "samples/count duration/nanoseconds" to describe the sample types + if n == 0 && looksLikeHeader(line) { + p.SampleType = nil + for _, sampleType := range strings.Split(line, " ") { + parts := strings.Split(sampleType, "/") + if len(parts) != 2 { + return nil, fmt.Errorf("bad header: %d: %q", n, line) + } + p.SampleType = append(p.SampleType, &profile.ValueType{ + Type: parts[0], + Unit: parts[1], + }) + } + continue + } + + parts := strings.Split(line, " ") + if len(parts) != len(p.SampleType)+1 { + return nil, fmt.Errorf("bad line: %d: %q", n, line) + } + + stack := strings.Split(parts[0], ";") + sample := &profile.Sample{} + for _, valS := range parts[1:] { + val, err := strconv.ParseInt(valS, 10, 64) + if err != nil { + return nil, fmt.Errorf("bad line: %d: %q: %s", n, line, err) + } + sample.Value = append(sample.Value, val) + } + + for i := range stack { + frame := stack[len(stack)-i-1] + function := &profile.Function{ + ID: functionID, + Name: frame, + } + p.Function = append(p.Function, function) + functionID++ + + location := &profile.Location{ + ID: locationID, + Mapping: m, + Line: []profile.Line{{Function: function}}, + } + p.Location = append(p.Location, location) + locationID++ + + sample.Location = append(sample.Location, location) + } + + p.Sample = append(p.Sample, sample) + } + return p, p.CheckValid() +} + +// looksLikeHeader returns true if the line looks like this: +// "samples/count duration/nanoseconds". The heuristic used for detecting this +// is to check if every space separated value contains a "/" character. +func looksLikeHeader(line string) bool { + for _, sampleType := range strings.Split(line, " ") { + if !strings.Contains(sampleType, "/") { + return false + } + } + return true +} diff --git a/profiler/internal/pprofutils/text_test.go b/profiler/internal/pprofutils/text_test.go new file mode 100644 index 0000000000..fc5a8453d2 --- /dev/null +++ b/profiler/internal/pprofutils/text_test.go @@ -0,0 +1,57 @@ +// 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 2021 Datadog, Inc. + +package pprofutils + +import ( + "bytes" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestTextConvert(t *testing.T) { + t.Run("simple", func(t *testing.T) { + textIn := strings.TrimSpace(` +main;foo 5 +main;foobar 4 +main;foo;bar 3 +`) + proto, err := Text{}.Convert(strings.NewReader(textIn)) + require.NoError(t, err) + textOut := bytes.Buffer{} + require.NoError(t, Protobuf{}.Convert(proto, &textOut)) + require.Equal(t, textIn+"\n", textOut.String()) + }) + + t.Run("headerWithOneSampleType", func(t *testing.T) { + textIn := strings.TrimSpace(` +samples/count +main;foo 5 +main;foobar 4 +main;foo;bar 3 + `) + proto, err := Text{}.Convert(strings.NewReader(textIn)) + require.NoError(t, err) + textOut := bytes.Buffer{} + require.NoError(t, Protobuf{SampleTypes: true}.Convert(proto, &textOut)) + require.Equal(t, textIn+"\n", textOut.String()) + }) + + t.Run("headerWithMultipleSampleTypes", func(t *testing.T) { + textIn := strings.TrimSpace(` +samples/count duration/nanoseconds +main;foo 5 50000000 +main;foobar 4 40000000 +main;foo;bar 3 30000000 + `) + proto, err := Text{}.Convert(strings.NewReader(textIn)) + require.NoError(t, err) + textOut := bytes.Buffer{} + require.NoError(t, Protobuf{SampleTypes: true}.Convert(proto, &textOut)) + require.Equal(t, textIn+"\n", textOut.String()) + }) +} diff --git a/profiler/profile.go b/profiler/profile.go index bb32323c71..cbde48b0bd 100644 --- a/profiler/profile.go +++ b/profiler/profile.go @@ -16,6 +16,7 @@ import ( "github.com/DataDog/gostackparse" pprofile "github.com/google/pprof/profile" + "gopkg.in/DataDog/dd-trace-go.v1/profiler/internal/pprofutils" ) // ProfileType represents a type of profile that the profiler is able to run. @@ -45,48 +46,139 @@ const ( MetricsProfile ) -func (t ProfileType) String() string { - switch t { - case HeapProfile: - return "heap" - case CPUProfile: - return "cpu" - case MutexProfile: - return "mutex" - case BlockProfile: - return "block" - case GoroutineProfile: - return "goroutine" - case expGoroutineWaitProfile: - return "goroutinewait" - case MetricsProfile: - return "metrics" - default: - return "unknown" +// profileType holds the implementation details of a ProfileType. +type profileType struct { + // Type gets populated automatically by ProfileType.lookup(). + Type ProfileType + // Name specifies the profile name as used with pprof.Lookup(name) (in + // collectGenericProfile) and returned by ProfileType.String(). For profile + // types that don't use this approach (e.g. CPU) the name isn't used for + // anything. + Name string + // Filename is the filename used for uploading the profile to the datadog + // backend which is aware of them. Delta profiles are prefixed with "delta-" + // automatically. In theory this could be derrived from the Name field, but + // this isn't done due to idiosyncratic filename used by the + // GoroutineProfile. + Filename string + // Delta controls if this profile should be generated as a delta profile. + // This is useful for profiles that represent samples collected over the + // lifetime of the process (i.e. heap, block, mutex). If nil, no delta + // profile is generated. + Delta *pprofutils.Delta + // Collect collects the given profile and returns the data for it. Most + // profiles will be in pprof format, i.e. gzip compressed proto buf data. + Collect func(profileType, *profiler) ([]byte, error) +} + +// profileTypes maps every ProfileType to its implementation. +var profileTypes = map[ProfileType]profileType{ + CPUProfile: { + Name: "cpu", + Filename: "cpu.pprof", + Collect: func(_ profileType, p *profiler) ([]byte, error) { + var buf bytes.Buffer + if err := startCPUProfile(&buf); err != nil { + return nil, err + } + p.interruptibleSleep(p.cfg.cpuDuration) + stopCPUProfile() + return buf.Bytes(), nil + }, + }, + // HeapProfile is complex due to how the Go runtime exposes it. It contains 4 + // sample types alloc_objects/count, alloc_space/bytes, inuse_objects/count, + // inuse_space/bytes. The first two represent allocations over the lifetime + // of the process, so we do delta profiling for them. The last two are + // snapshots of the current heap state, so we leave them as-is. + HeapProfile: { + Name: "heap", + Filename: "heap.pprof", + Delta: &pprofutils.Delta{SampleTypes: []pprofutils.ValueType{ + {Type: "alloc_objects", Unit: "count"}, + {Type: "alloc_space", Unit: "bytes"}, + }}, + Collect: collectGenericProfile, + }, + MutexProfile: { + Name: "mutex", + Filename: "mutex.pprof", + Delta: &pprofutils.Delta{}, + Collect: collectGenericProfile, + }, + BlockProfile: { + Name: "block", + Filename: "block.pprof", + Delta: &pprofutils.Delta{}, + Collect: collectGenericProfile, + }, + GoroutineProfile: { + Name: "goroutine", + Filename: "goroutines.pprof", + Collect: collectGenericProfile, + }, + expGoroutineWaitProfile: { + Name: "goroutinewait", + Filename: "goroutineswait.pprof", + Collect: func(t profileType, p *profiler) ([]byte, error) { + if n := runtime.NumGoroutine(); n > p.cfg.maxGoroutinesWait { + return nil, fmt.Errorf("skipping goroutines wait profile: %d goroutines exceeds DD_PROFILING_WAIT_PROFILE_MAX_GOROUTINES limit of %d", n, p.cfg.maxGoroutinesWait) + } + + var ( + now = now() + text = &bytes.Buffer{} + pprof = &bytes.Buffer{} + ) + if err := lookupProfile(t.Name, text, 2); err != nil { + return nil, err + } + err := goroutineDebug2ToPprof(text, pprof, now) + return pprof.Bytes(), err + }, + }, + MetricsProfile: { + Name: "metrics", + Filename: "metrics.json", + Collect: func(_ profileType, p *profiler) ([]byte, error) { + var buf bytes.Buffer + err := p.met.report(now(), &buf) + return buf.Bytes(), err + }, + }, +} + +func collectGenericProfile(t profileType, _ *profiler) ([]byte, error) { + var buf bytes.Buffer + err := lookupProfile(t.Name, &buf, 0) + return buf.Bytes(), err +} + +// lookup returns t's profileType implementation. +func (t ProfileType) lookup() profileType { + c, ok := profileTypes[t] + if ok { + c.Type = t + return c } + return profileType{ + Type: t, + Name: "unknown", + Filename: "unknown", + Collect: func(_ profileType, _ *profiler) ([]byte, error) { + return nil, errors.New("profile type not implemented") + }, + } +} + +// String returns the name of the profile. +func (t ProfileType) String() string { + return t.lookup().Name } // Filename is the identifier used on upload. func (t ProfileType) Filename() string { - // There are subtle differences between the root and String() (see GoroutineProfile) - switch t { - case HeapProfile: - return "heap.pprof" - case CPUProfile: - return "cpu.pprof" - case MutexProfile: - return "mutex.pprof" - case BlockProfile: - return "block.pprof" - case GoroutineProfile: - return "goroutines.pprof" - case expGoroutineWaitProfile: - return "goroutineswait.pprof" - case MetricsProfile: - return "metrics.json" - default: - return "unknown" - } + return t.lookup().Filename } // Tag used on profile metadata @@ -113,42 +205,80 @@ func (b *batch) addProfile(p *profile) { b.profiles = append(b.profiles, p) } -func (p *profiler) runProfile(t ProfileType) (*profile, error) { - switch t { - case HeapProfile: - return heapProfile(p.cfg) - case CPUProfile: - return p.cpuProfile() - case MutexProfile: - return mutexProfile(p.cfg) - case BlockProfile: - return blockProfile(p.cfg) - case GoroutineProfile: - return goroutineProfile(p.cfg) - case expGoroutineWaitProfile: - return goroutineWaitProfile(p.cfg) - case MetricsProfile: - return p.collectMetrics() - default: - return nil, errors.New("profile type not implemented") - } -} - -// writeHeapProfile writes the heap profile; replaced in tests -var writeHeapProfile = pprof.WriteHeapProfile - -func heapProfile(cfg *config) (*profile, error) { - var buf bytes.Buffer +func (p *profiler) runProfile(pt ProfileType) ([]*profile, error) { start := now() - if err := writeHeapProfile(&buf); err != nil { + t := pt.lookup() + // Collect the original profile as-is. + data, err := t.Collect(t, p) + if err != nil { return nil, err } + profs := []*profile{{ + name: t.Filename, + data: data, + }} + // Compute the deltaProf (will be nil if not enabled for this profile type). + deltaStart := time.Now() + deltaProf, err := p.deltaProfile(t, data) + if err != nil { + return nil, fmt.Errorf("delta profile error: %s", err) + } + // Report metrics and append deltaProf if not nil. end := now() - tags := append(cfg.tags, HeapProfile.Tag()) - cfg.statsd.Timing("datadog.profiler.go.collect_time", end.Sub(start), tags, 1) + tags := append(p.cfg.tags, pt.Tag()) + // TODO(fg) stop uploading non-delta profiles in the next version of + // dd-trace-go after delta profiles are released. + if deltaProf != nil { + profs = append(profs, deltaProf) + p.cfg.statsd.Timing("datadog.profiler.go.delta_time", end.Sub(deltaStart), tags, 1) + } + p.cfg.statsd.Timing("datadog.profiler.go.collect_time", end.Sub(start), tags, 1) + return profs, nil +} + +// deltaProfile derives the delta profile between curData and the previous +// profile. For profile types that don't have delta profiling enabled, it +// simply returns nil, nil. +func (p *profiler) deltaProfile(t profileType, curData []byte) (*profile, error) { + // Not all profile types use delta profiling, return nil if this one doesn't. + if t.Delta == nil { + return nil, nil + } + curProf, err := pprofile.ParseData(curData) + if err != nil { + return nil, fmt.Errorf("delta prof parse: %v", err) + } + var deltaData []byte + if prevProf := p.prev[t.Type]; prevProf == nil { + // First time deltaProfile gets called for a type, there is no prevProf. In + // this case we emit the current profile as a delta profile. + deltaData = curData + } else { + // Delta profiling is also implemented in the Go core, see commit below. + // Unfortunately the core implementation isn't resuable via a API, so we do + // our own delta calculation below. + // https://github.com/golang/go/commit/2ff1e3ebf5de77325c0e96a6c2a229656fc7be50#diff-94594f8f13448da956b02997e50ca5a156b65085993e23bbfdda222da6508258R303-R304 + deltaProf, err := t.Delta.Convert(prevProf, curProf) + if err != nil { + return nil, fmt.Errorf("delta prof merge: %v", err) + } + // TimeNanos is supposed to be the time the profile was collected, see + // https://github.com/google/pprof/blob/master/proto/profile.proto. + deltaProf.TimeNanos = curProf.TimeNanos + // DurationNanos is the time period covered by the profile. + deltaProf.DurationNanos = curProf.TimeNanos - prevProf.TimeNanos + deltaBuf := &bytes.Buffer{} + if err := deltaProf.Write(deltaBuf); err != nil { + return nil, fmt.Errorf("delta prof write: %v", err) + } + deltaData = deltaBuf.Bytes() + } + // Keep the most recent profiles in memory for future diffing. This needs to + // be taken into account when enforcing memory limits going forward. + p.prev[t.Type] = curProf return &profile{ - name: HeapProfile.Filename(), - data: buf.Bytes(), + name: "delta-" + t.Filename, + data: deltaData, }, nil } @@ -159,23 +289,6 @@ var ( stopCPUProfile = pprof.StopCPUProfile ) -func (p *profiler) cpuProfile() (*profile, error) { - var buf bytes.Buffer - start := now() - if err := startCPUProfile(&buf); err != nil { - return nil, err - } - p.interruptibleSleep(p.cfg.cpuDuration) - stopCPUProfile() - end := now() - tags := append(p.cfg.tags, CPUProfile.Tag()) - p.cfg.statsd.Timing("datadog.profiler.go.collect_time", end.Sub(start), tags, 1) - return &profile{ - name: CPUProfile.Filename(), - data: buf.Bytes(), - }, nil -} - // lookpupProfile looks up the profile with the given name and writes it to w. It returns // any errors encountered in the process. It is replaced in tests. var lookupProfile = func(name string, w io.Writer, debug int) error { @@ -186,76 +299,6 @@ var lookupProfile = func(name string, w io.Writer, debug int) error { return prof.WriteTo(w, debug) } -func blockProfile(cfg *config) (*profile, error) { - var buf bytes.Buffer - start := now() - if err := lookupProfile(BlockProfile.String(), &buf, 0); err != nil { - return nil, err - } - end := now() - tags := append(cfg.tags, BlockProfile.Tag()) - cfg.statsd.Timing("datadog.profiler.go.collect_time", end.Sub(start), tags, 1) - return &profile{ - name: BlockProfile.Filename(), - data: buf.Bytes(), - }, nil -} - -func mutexProfile(cfg *config) (*profile, error) { - var buf bytes.Buffer - start := now() - if err := lookupProfile(MutexProfile.String(), &buf, 0); err != nil { - return nil, err - } - end := now() - tags := append(cfg.tags, MutexProfile.Tag()) - cfg.statsd.Timing("datadog.profiler.go.collect_time", end.Sub(start), tags, 1) - return &profile{ - name: MutexProfile.Filename(), - data: buf.Bytes(), - }, nil -} - -func goroutineProfile(cfg *config) (*profile, error) { - var buf bytes.Buffer - start := now() - if err := lookupProfile(GoroutineProfile.String(), &buf, 0); err != nil { - return nil, err - } - end := now() - tags := append(cfg.tags, GoroutineProfile.Tag()) - cfg.statsd.Timing("datadog.profiler.go.collect_time", end.Sub(start), tags, 1) - return &profile{ - name: GoroutineProfile.Filename(), - data: buf.Bytes(), - }, nil -} - -func goroutineWaitProfile(cfg *config) (*profile, error) { - if n := runtime.NumGoroutine(); n > cfg.maxGoroutinesWait { - return nil, fmt.Errorf("skipping goroutines wait profile: %d goroutines exceeds DD_PROFILING_WAIT_PROFILE_MAX_GOROUTINES limit of %d", n, cfg.maxGoroutinesWait) - } - - var ( - text = &bytes.Buffer{} - pprof = &bytes.Buffer{} - start = now() - ) - if err := lookupProfile(GoroutineProfile.String(), text, 2); err != nil { - return nil, err - } else if err := goroutineDebug2ToPprof(text, pprof, start); err != nil { - return nil, err - } - end := now() - tags := append(cfg.tags, expGoroutineWaitProfile.Tag()) - cfg.statsd.Timing("datadog.profiler.go.collect_time", end.Sub(start), tags, 1) - - return &profile{ - name: expGoroutineWaitProfile.Filename(), - data: pprof.Bytes(), - }, nil -} - func goroutineDebug2ToPprof(r io.Reader, w io.Writer, t time.Time) (err error) { // gostackparse.Parse() has been extensively tested and should not crash // under any circumstances, but we really want to avoid crashing a customers @@ -353,21 +396,6 @@ func goroutineDebug2ToPprof(r io.Reader, w io.Writer, t time.Time) (err error) { return nil } -func (p *profiler) collectMetrics() (*profile, error) { - var buf bytes.Buffer - start := now() - if err := p.met.report(start, &buf); err != nil { - return nil, err - } - end := now() - tags := append(p.cfg.tags, MetricsProfile.Tag()) - p.cfg.statsd.Timing("datadog.profiler.go.collect_time", end.Sub(start), tags, 1) - return &profile{ - name: MetricsProfile.Filename(), - data: buf.Bytes(), - }, nil -} - // now returns current time in UTC. func now() time.Time { return time.Now().UTC() diff --git a/profiler/profile_test.go b/profiler/profile_test.go index 74174f59f0..5e45a4242c 100644 --- a/profiler/profile_test.go +++ b/profiler/profile_test.go @@ -12,26 +12,147 @@ import ( "io/ioutil" "os" "strconv" + "strings" "testing" "time" pprofile "github.com/google/pprof/profile" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gopkg.in/DataDog/dd-trace-go.v1/profiler/internal/pprofutils" ) func TestRunProfile(t *testing.T) { - t.Run("heap", func(t *testing.T) { - defer func(old func(_ io.Writer) error) { writeHeapProfile = old }(writeHeapProfile) - writeHeapProfile = func(w io.Writer) error { - _, err := w.Write([]byte("my-heap-profile")) - return err + t.Run("delta", func(t *testing.T) { + var ( + deltaPeriod = DefaultPeriod + timeA = time.Now().Truncate(time.Minute) + timeB = timeA.Add(deltaPeriod) + ) + + tests := []struct { + Types []ProfileType + Prof1 textProfile + Prof2 textProfile + WantDelta textProfile + WantDuration time.Duration + }{ + // For the mutex and block profile, we derive the delta for all sample + // types, so we can test with a generic sample profile. + { + Types: []ProfileType{MutexProfile, BlockProfile}, + Prof1: textProfile{ + Time: timeA, + Text: ` +stuff/count +main 3 +main;bar 2 +main;foo 5 +`, + }, + Prof2: textProfile{ + Time: timeB, + Text: ` +stuff/count +main 4 +main;bar 2 +main;foo 8 +main;foobar 7 +`, + }, + WantDelta: textProfile{ + Time: timeA, + Text: ` +stuff/count +main;foobar 7 +main;foo 3 +main 1 +`, + }, + WantDuration: deltaPeriod, + }, + + // For the heap profile, we must only derive deltas for the + // alloc_objects/count and alloc_space/bytes sample type, so we use a + // more realistic example and make sure it is handled accurately. + { + Types: []ProfileType{HeapProfile}, + Prof1: textProfile{ + Time: timeA, + Text: ` +alloc_objects/count alloc_space/bytes inuse_objects/count inuse_space/bytes +main 3 6 12 24 +main;bar 2 4 8 16 +main;foo 5 10 20 40 +`, + }, + Prof2: textProfile{ + Time: timeB, + Text: ` +alloc_objects/count alloc_space/bytes inuse_objects/count inuse_space/bytes +main 4 8 16 32 +main;bar 2 4 8 16 +main;foo 8 16 32 64 +main;foobar 7 14 28 56 +`, + }, + WantDelta: textProfile{ + Time: timeA, + Text: ` +alloc_objects/count alloc_space/bytes inuse_objects/count inuse_space/bytes +main;foobar 7 14 28 56 +main;foo 3 6 32 64 +main 1 2 16 32 +main;bar 0 0 8 16 +`, + }, + WantDuration: deltaPeriod, + }, + } + + for _, test := range tests { + for _, profType := range test.Types { + t.Run(profType.String(), func(t *testing.T) { + prof1 := test.Prof1.Protobuf() + prof2 := test.Prof2.Protobuf() + + returnProfs := [][]byte{prof1, prof2} + defer func(old func(_ string, _ io.Writer, _ int) error) { lookupProfile = old }(lookupProfile) + lookupProfile = func(name string, w io.Writer, _ int) error { + _, err := w.Write(returnProfs[0]) + returnProfs = returnProfs[1:] + return err + } + p, err := unstartedProfiler() + + // first run, should produce the current profile twice (a bit + // awkward, but makes sense since we try to add delta profiles as an + // additional profile type to ease the transition) + profs, err := p.runProfile(profType) + require.NoError(t, err) + require.Equal(t, 2, len(profs)) + require.Equal(t, profType.Filename(), profs[0].name) + require.Equal(t, prof1, profs[0].data) + require.Equal(t, "delta-"+profType.Filename(), profs[1].name) + require.Equal(t, prof1, profs[1].data) + + // second run, should produce p1 profile and delta profile + profs, err = p.runProfile(profType) + require.NoError(t, err) + require.Equal(t, 2, len(profs)) + require.Equal(t, profType.Filename(), profs[0].name) + require.Equal(t, prof2, profs[0].data) + require.Equal(t, "delta-"+profType.Filename(), profs[1].name) + require.Equal(t, test.WantDelta.String(), protobufToText(profs[1].data)) + + // check delta prof details like timestamps and duration + deltaProf, err := pprofile.ParseData(profs[1].data) + require.NoError(t, err) + require.Equal(t, test.Prof2.Time.UnixNano(), deltaProf.TimeNanos) + require.Equal(t, deltaPeriod.Nanoseconds(), deltaProf.DurationNanos) + }) + } } - p, err := unstartedProfiler() - prof, err := p.runProfile(HeapProfile) - require.NoError(t, err) - assert.Equal(t, "heap.pprof", prof.name) - assert.Equal(t, []byte("my-heap-profile"), prof.data) }) t.Run("cpu", func(t *testing.T) { @@ -45,40 +166,12 @@ func TestRunProfile(t *testing.T) { p, err := unstartedProfiler(CPUDuration(10 * time.Millisecond)) start := time.Now() - prof, err := p.runProfile(CPUProfile) + profs, err := p.runProfile(CPUProfile) end := time.Now() require.NoError(t, err) assert.True(t, end.Sub(start) > 10*time.Millisecond) - assert.Equal(t, "cpu.pprof", prof.name) - assert.Equal(t, []byte("my-cpu-profile"), prof.data) - }) - - t.Run("mutex", func(t *testing.T) { - defer func(old func(_ string, _ io.Writer, _ int) error) { lookupProfile = old }(lookupProfile) - lookupProfile = func(name string, w io.Writer, _ int) error { - _, err := w.Write([]byte(name)) - return err - } - - p, err := unstartedProfiler() - prof, err := p.runProfile(MutexProfile) - require.NoError(t, err) - assert.Equal(t, "mutex.pprof", prof.name) - assert.Equal(t, []byte("mutex"), prof.data) - }) - - t.Run("block", func(t *testing.T) { - defer func(old func(_ string, _ io.Writer, _ int) error) { lookupProfile = old }(lookupProfile) - lookupProfile = func(name string, w io.Writer, _ int) error { - _, err := w.Write([]byte(name)) - return err - } - - p, err := unstartedProfiler() - prof, err := p.runProfile(BlockProfile) - require.NoError(t, err) - assert.Equal(t, "block.pprof", prof.name) - assert.Equal(t, []byte("block"), prof.data) + assert.Equal(t, "cpu.pprof", profs[0].name) + assert.Equal(t, []byte("my-cpu-profile"), profs[0].data) }) t.Run("goroutine", func(t *testing.T) { @@ -89,10 +182,10 @@ func TestRunProfile(t *testing.T) { } p, err := unstartedProfiler() - prof, err := p.runProfile(GoroutineProfile) + profs, err := p.runProfile(GoroutineProfile) require.NoError(t, err) - assert.Equal(t, "goroutines.pprof", prof.name) - assert.Equal(t, []byte("goroutine"), prof.data) + assert.Equal(t, "goroutines.pprof", profs[0].name) + assert.Equal(t, []byte("goroutine"), profs[0].data) }) t.Run("goroutinewait", func(t *testing.T) { @@ -125,9 +218,9 @@ main.main() } p, err := unstartedProfiler() - prof, err := p.runProfile(expGoroutineWaitProfile) + profs, err := p.runProfile(expGoroutineWaitProfile) require.NoError(t, err) - require.Equal(t, "goroutineswait.pprof", prof.name) + require.Equal(t, "goroutineswait.pprof", profs[0].name) // pro tip: enable line below to inspect the pprof output using cli tools // ioutil.WriteFile(prof.name, prof.data, 0644) @@ -141,7 +234,7 @@ main.main() require.Equal(t, want, got) } - pp, err := pprofile.Parse(bytes.NewReader(prof.data)) + pp, err := pprofile.Parse(bytes.NewReader(profs[0].data)) require.NoError(t, err) // timestamp require.NotEqual(t, int64(0), pp.TimeNanos) @@ -231,3 +324,68 @@ type panicReader struct{} func (c panicReader) Read(_ []byte) (int, error) { panic("42") } + +// textProfile is a test helper for converting folded text to pprof protobuf +// profiles. +// See https://github.com/brendangregg/FlameGraph#2-fold-stacks +type textProfile struct { + Text string + Time time.Time +} + +// Protobuf converts the profile to pprof's protobuf format or panics if there +// is an error. +func (t textProfile) Protobuf() []byte { + out := &bytes.Buffer{} + prof, err := pprofutils.Text{}.Convert(strings.NewReader(t.String())) + if err != nil { + panic(err) + } + if !t.Time.IsZero() { + prof.TimeNanos = t.Time.UnixNano() + } + if err := prof.Write(out); err != nil { + panic(err) + } + return out.Bytes() +} + +// String returns text without leading or trailing whitespace other than a +// trailing newline. +func (t textProfile) String() string { + return strings.TrimSpace(t.Text) + "\n" +} + +// protobufToText is a test helper that converts a protobuf pprof profile to +// text format or panics if there is an error. +func protobufToText(pprofData []byte) string { + prof, err := pprofile.ParseData(pprofData) + if err != nil { + panic(err) + } + out := &bytes.Buffer{} + if err := (pprofutils.Protobuf{SampleTypes: true}).Convert(prof, out); err != nil { + panic(err) + } + return out.String() +} + +// TestProfileTypeSoundness fails if somebody tries to add a new profile type +// without adding it to enabledProfileTypes as well. +func TestProfileTypeSoundness(t *testing.T) { + t.Run("enabledProfileTypes", func(t *testing.T) { + var allProfileTypes []ProfileType + for pt := range profileTypes { + allProfileTypes = append(allProfileTypes, pt) + } + p, err := unstartedProfiler(WithProfileTypes(allProfileTypes...)) + require.NoError(t, err) + types := p.enabledProfileTypes() + require.Equal(t, len(allProfileTypes), len(types)) + }) + + t.Run("profileTypes", func(t *testing.T) { + _, err := unstartedProfiler(WithProfileTypes(ProfileType(-1))) + require.EqualError(t, err, "unknown profile type: -1") + }) +} diff --git a/profiler/profiler.go b/profiler/profiler.go index 7543eb9f0b..52fda22a9b 100644 --- a/profiler/profiler.go +++ b/profiler/profiler.go @@ -15,6 +15,7 @@ import ( "sync" "time" + pprofile "github.com/google/pprof/profile" "gopkg.in/DataDog/dd-trace-go.v1/internal" "gopkg.in/DataDog/dd-trace-go.v1/internal/log" ) @@ -59,13 +60,14 @@ func Stop() { // profiler collects and sends preset profiles to the Datadog API at a given frequency // using a given configuration. type profiler struct { - cfg *config // profile configuration - out chan batch // upload queue - uploadFunc func(batch) error // defaults to (*profiler).upload; replaced in tests - exit chan struct{} // exit signals the profiler to stop; it is closed after stopping - stopOnce sync.Once // stopOnce ensures the profiler is stopped exactly once. - wg sync.WaitGroup // wg waits for all goroutines to exit when stopping. - met *metrics // metric collector state + cfg *config // profile configuration + out chan batch // upload queue + uploadFunc func(batch) error // defaults to (*profiler).upload; replaced in tests + exit chan struct{} // exit signals the profiler to stop; it is closed after stopping + stopOnce sync.Once // stopOnce ensures the profiler is stopped exactly once. + wg sync.WaitGroup // wg waits for all goroutines to exit when stopping. + met *metrics // metric collector state + prev map[ProfileType]*pprofile.Profile // previous collection results for delta profiling } // newProfiler creates a new, unstarted profiler. @@ -123,11 +125,18 @@ func newProfiler(opts ...Option) (*profiler, error) { if cfg.uploadTimeout <= 0 { return nil, fmt.Errorf("invalid upload timeout, must be > 0: %s", cfg.uploadTimeout) } + for pt := range cfg.types { + if _, ok := profileTypes[pt]; !ok { + return nil, fmt.Errorf("unknown profile type: %d", pt) + } + } + p := profiler{ cfg: cfg, out: make(chan batch, outChannelSize), exit: make(chan struct{}), met: newMetrics(), + prev: make(map[ProfileType]*pprofile.Profile), } p.uploadFunc = p.upload return &p, nil @@ -173,14 +182,17 @@ func (p *profiler) collect(ticker <-chan time.Time) { // configured CPU profile duration: (start-end). end: now.Add(p.cfg.cpuDuration), } - for t := range p.cfg.types { - prof, err := p.runProfile(t) + + for _, t := range p.enabledProfileTypes() { + profs, err := p.runProfile(t) if err != nil { log.Error("Error getting %s profile: %v; skipping.", t, err) p.cfg.statsd.Count("datadog.profiler.go.collect_error", 1, append(p.cfg.tags, t.Tag()), 1) continue } - bat.addProfile(prof) + for _, prof := range profs { + bat.addProfile(prof) + } } p.enqueueUpload(bat) case <-p.exit: @@ -189,6 +201,30 @@ func (p *profiler) collect(ticker <-chan time.Time) { } } +// enabledProfileTypes returns the enabled profile types in a deterministic +// order. The CPU profile always comes first because people might spot +// interesting events in there and then try to look for the counter-part event +// in the mutex/heap/block profile. Deterministic ordering is also important +// for delta profiles, otherwise they'd cover varying profiling periods. +func (p *profiler) enabledProfileTypes() []ProfileType { + order := []ProfileType{ + CPUProfile, + HeapProfile, + BlockProfile, + MutexProfile, + GoroutineProfile, + expGoroutineWaitProfile, + MetricsProfile, + } + enabled := []ProfileType{} + for _, t := range order { + if _, ok := p.cfg.types[t]; ok { + enabled = append(enabled, t) + } + } + return enabled +} + // enqueueUpload pushes a batch of profiles onto the queue to be uploaded. If there is no room, it will // evict the oldest profile to make some. Typically a batch would be one of each enabled profile. func (p *profiler) enqueueUpload(bat batch) { diff --git a/profiler/profiler_test.go b/profiler/profiler_test.go index cec69d3fe7..80232a82ab 100644 --- a/profiler/profiler_test.go +++ b/profiler/profiler_test.go @@ -210,11 +210,15 @@ func TestProfilerInternal(t *testing.T) { } defer func(old func()) { stopCPUProfile = old }(stopCPUProfile) stopCPUProfile = func() { atomic.AddUint64(&stopCPU, 1) } - defer func(old func(_ io.Writer) error) { writeHeapProfile = old }(writeHeapProfile) - writeHeapProfile = func(_ io.Writer) error { - atomic.AddUint64(&writeHeap, 1) - return nil + defer func(old func(_ string, w io.Writer, _ int) error) { lookupProfile = old }(lookupProfile) + lookupProfile = func(name string, w io.Writer, _ int) error { + if name == "heap" { + atomic.AddUint64(&writeHeap, 1) + } + _, err := w.Write(textProfile{Text: "main 5\n"}.Protobuf()) + return err } + tick := make(chan time.Time) wait := make(chan struct{}) @@ -237,7 +241,8 @@ func TestProfilerInternal(t *testing.T) { assert.EqualValues(1, startCPU) assert.EqualValues(1, stopCPU) - assert.Equal(3, len(bat.profiles)) + // should contain cpu.pprof, metrics.json, heap.pprof, delta-heap.pprof + assert.Equal(4, len(bat.profiles)) p.exit <- struct{}{} <-wait @@ -290,9 +295,11 @@ func TestProfilerPassthrough(t *testing.T) { } assert := assert.New(t) - assert.Equal(2, len(bat.profiles)) + // should contain cpu.pprof, heap.pprof, delta-heap.pprof + assert.Equal(3, len(bat.profiles)) assert.NotEmpty(bat.profiles[0].data) assert.NotEmpty(bat.profiles[1].data) + assert.NotEmpty(bat.profiles[2].data) } func unstartedProfiler(opts ...Option) (*profiler, error) { From 4cc670944564d33a6bca2cefa21226de0ef800f1 Mon Sep 17 00:00:00 2001 From: Kyle Nusbaum Date: Wed, 25 Aug 2021 21:03:51 -0500 Subject: [PATCH 6/8] profiler: fix imports (#986) --- profiler/profile.go | 3 ++- profiler/profile_test.go | 3 ++- profiler/profiler.go | 1 + 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/profiler/profile.go b/profiler/profile.go index cbde48b0bd..eb51a52a77 100644 --- a/profiler/profile.go +++ b/profiler/profile.go @@ -14,9 +14,10 @@ import ( "runtime/pprof" "time" + "gopkg.in/DataDog/dd-trace-go.v1/profiler/internal/pprofutils" + "github.com/DataDog/gostackparse" pprofile "github.com/google/pprof/profile" - "gopkg.in/DataDog/dd-trace-go.v1/profiler/internal/pprofutils" ) // ProfileType represents a type of profile that the profiler is able to run. diff --git a/profiler/profile_test.go b/profiler/profile_test.go index 5e45a4242c..b279cde6cf 100644 --- a/profiler/profile_test.go +++ b/profiler/profile_test.go @@ -16,10 +16,11 @@ import ( "testing" "time" + "gopkg.in/DataDog/dd-trace-go.v1/profiler/internal/pprofutils" + pprofile "github.com/google/pprof/profile" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "gopkg.in/DataDog/dd-trace-go.v1/profiler/internal/pprofutils" ) func TestRunProfile(t *testing.T) { diff --git a/profiler/profiler.go b/profiler/profiler.go index 52fda22a9b..0f3c65a88a 100644 --- a/profiler/profiler.go +++ b/profiler/profiler.go @@ -16,6 +16,7 @@ import ( "time" pprofile "github.com/google/pprof/profile" + "gopkg.in/DataDog/dd-trace-go.v1/internal" "gopkg.in/DataDog/dd-trace-go.v1/internal/log" ) From 04e152188c1a010f00acd213f10b1328dae28021 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eduardo=20Bonzi=20da=20Concei=C3=A7=C3=A3o?= Date: Mon, 30 Aug 2021 15:02:20 -0300 Subject: [PATCH 7/8] contrib/gofiber/fiber.v2: capture error from fiber handler (#988) The error returned from the fiber handler was being ignored by the middleware. It now correctly captures the error and sets it on the span. Fixes #978 --- contrib/gofiber/fiber.v2/fiber.go | 4 +++- contrib/gofiber/fiber.v2/fiber_test.go | 32 +++++++++++++++++++++++++- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/contrib/gofiber/fiber.v2/fiber.go b/contrib/gofiber/fiber.v2/fiber.go index 24da04ab92..e664deda00 100644 --- a/contrib/gofiber/fiber.v2/fiber.go +++ b/contrib/gofiber/fiber.v2/fiber.go @@ -64,7 +64,9 @@ func Middleware(opts ...Option) func(c *fiber.Ctx) error { } span.SetTag(ext.HTTPCode, strconv.Itoa(status)) - if cfg.isStatusError(status) { + if err != nil { + span.SetTag(ext.Error, err) + } else if cfg.isStatusError(status) { // mark 5xx server error span.SetTag(ext.Error, fmt.Errorf("%d: %s", status, http.StatusText(status))) } diff --git a/contrib/gofiber/fiber.v2/fiber_test.go b/contrib/gofiber/fiber.v2/fiber_test.go index 4b8d0c10ca..97b3ae1242 100644 --- a/contrib/gofiber/fiber.v2/fiber_test.go +++ b/contrib/gofiber/fiber.v2/fiber_test.go @@ -94,7 +94,7 @@ func TestTrace200(t *testing.T) { }) } -func TestError(t *testing.T) { +func TestStatusError(t *testing.T) { assert := assert.New(t) mt := mocktracer.Start() defer mt.Stop() @@ -128,6 +128,36 @@ func TestError(t *testing.T) { assert.Equal(wantErr, span.Tag(ext.Error).(error).Error()) } +func TestCustomError(t *testing.T) { + assert := assert.New(t) + mt := mocktracer.Start() + defer mt.Stop() + + router := fiber.New() + router.Use(Middleware(WithServiceName("foobar"))) + + router.Get("/err", func(c *fiber.Ctx) error { + c.SendStatus(400) + return fiber.ErrBadRequest + }) + r := httptest.NewRequest("GET", "/err", nil) + + response, err := router.Test(r, 100) + assert.Equal(nil, err) + assert.Equal(response.StatusCode, 400) + + spans := mt.FinishedSpans() + assert.Len(spans, 1) + if len(spans) < 1 { + t.Fatalf("no spans") + } + span := spans[0] + assert.Equal("http.request", span.OperationName()) + assert.Equal("foobar", span.Tag(ext.ServiceName)) + assert.Equal("400", span.Tag(ext.HTTPCode)) + assert.Equal(fiber.ErrBadRequest, span.Tag(ext.Error).(*fiber.Error)) +} + func TestGetSpanNotInstrumented(t *testing.T) { assert := assert.New(t) router := fiber.New() From 9f17fc704a2fadb0e8fef789eb17805bd510fcf4 Mon Sep 17 00:00:00 2001 From: Kyle Nusbaum Date: Mon, 30 Aug 2021 14:26:56 -0500 Subject: [PATCH 8/8] internal/version: bump to v1.34.0 (#994) --- internal/version/version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/version/version.go b/internal/version/version.go index 6f2bc919cb..342505df5c 100644 --- a/internal/version/version.go +++ b/internal/version/version.go @@ -8,4 +8,4 @@ package version // Tag specifies the current release tag. It needs to be manually // updated. A test checks that the value of Tag never points to a // git tag that is older than HEAD. -const Tag = "v1.33.0" +const Tag = "v1.34.0"