diff --git a/.circleci/config.yml b/.circleci/config.yml index 1fb6f99290..7f79646b05 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -60,6 +60,16 @@ jobs: exit 1 fi + - run: + name: goimports + command: | + 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 + - run: name: lint command: | 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) 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/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() 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/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/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/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/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/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. 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" ) 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/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" 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 0000000000..1b4edefa7b Binary files /dev/null and b/profiler/internal/pprofutils/test-fixtures/pprof.lines.pb.gz differ diff --git a/profiler/internal/pprofutils/test-fixtures/pprof.samples.cpu.001.pb.gz b/profiler/internal/pprofutils/test-fixtures/pprof.samples.cpu.001.pb.gz new file mode 100644 index 0000000000..993d929518 Binary files /dev/null and b/profiler/internal/pprofutils/test-fixtures/pprof.samples.cpu.001.pb.gz differ 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/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" ) diff --git a/profiler/profile.go b/profiler/profile.go index bb32323c71..eb51a52a77 100644 --- a/profiler/profile.go +++ b/profiler/profile.go @@ -14,6 +14,8 @@ import ( "runtime/pprof" "time" + "gopkg.in/DataDog/dd-trace-go.v1/profiler/internal/pprofutils" + "github.com/DataDog/gostackparse" pprofile "github.com/google/pprof/profile" ) @@ -45,48 +47,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 +206,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 +290,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 +300,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 +397,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..b279cde6cf 100644 --- a/profiler/profile_test.go +++ b/profiler/profile_test.go @@ -12,26 +12,148 @@ import ( "io/ioutil" "os" "strconv" + "strings" "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" ) 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 +167,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 +183,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 +219,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 +235,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 +325,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..0f3c65a88a 100644 --- a/profiler/profiler.go +++ b/profiler/profiler.go @@ -15,6 +15,8 @@ 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 +61,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 +126,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 +183,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 +202,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) {