Merge branch 'v1' into knusbaum/bump-sketches-go

.circleci/config.yml

@@ -44,8 +44,21 @@ jobs:
     - run:
         name: go.sum up-to-date
         command: |
-          go run -mod=readonly gosum.go
-
+          if ! go run -mod=readonly gosum.go; then
+            # Older go versions, e.g. go1.14 will fail the check above with
+            # the message below.
+            #
+            #   go: updates to go.sum needed, disabled by -mod=readonly
+            #
+            # Newer versions show which go.sum entries are missing. To get
+            # useful CI errors for older versions, always print an explicit
+            # diff when go.sum is not up-to-date.
+            cp go.sum go.sum.before
+            go mod tidy
+            echo "--> go.sum diff:"
+            diff go.sum.before go.sum
+            exit 1
+          fi
     - run:
         name: milestone
         command: |
@@ -105,6 +118,11 @@ jobs:
       - restore_cache: # restores saved cache if no changes are detected since last run
           keys:
             - go-mod-v5-core-{{ checksum "go.sum.orig" }}
+      - run:
+          name: Enforce some dependencies
+          command: |
+            # last version compatible with go1.14, needed for testtraceprof
+            echo 'replace golang.org/x/net => golang.org/x/net d418f374d30933c6c7db22cf349625c295a5afaa' >> go.mod
       - run:
           name: Testing
           command: |
@@ -143,6 +161,8 @@ jobs:
         environment:
           GOPATH: "/home/circleci/go"
       - image: cassandra:3.7
+        environment:
+          JVM_OPTS: "-Xms750m -Xmx750m"
       - image: circleci/mysql:5.7
         environment:
           MYSQL_ROOT_PASSWORD: admin
@@ -165,6 +185,7 @@ jobs:
       - image: elasticsearch:6.8.13
         environment:
           http.port: 9202-9300
+          discovery.type: single-node
           ES_JAVA_OPTS: "-Xms750m -Xmx750m" # https://github.com/10up/wp-local-docker/issues/6
       - image: elasticsearch:7.14.1
         environment:
@@ -277,7 +298,9 @@ jobs:
           name: Testing integrations
           command: |
             PACKAGE_NAMES=$(go list ./contrib/... | grep -v -e grpc.v12 -e google.golang.org/api | circleci tests split --split-by=timings --timings-type=classname)
-            env DD_APPSEC_ENABLED=$(test "<< parameters.build_tags >>" = appsec && echo -n true) gotestsum --junitfile ${TEST_RESULTS}/gotestsum-report.xml -- $PACKAGE_NAMES -v -race -coverprofile=coverage.txt -covermode=atomic -tags "<< parameters.build_tags >>"
+            export DD_APPSEC_ENABLED=$(test "<< parameters.build_tags >>" = appsec && echo -n true)
+            export INTEGRATION=true
+            gotestsum --junitfile ${TEST_RESULTS}/gotestsum-report.xml -- $PACKAGE_NAMES -v -race -coverprofile=coverage.txt -covermode=atomic -tags "<< parameters.build_tags >>"
 
       - store_artifacts: # upload test summary for display in Artifacts
           path: /tmp/test-results
@@ -309,7 +332,7 @@ jobs:
                 git clone git@github.com:DataDog/sketches-go && cd sketches-go
                 git fetch origin && git checkout v1.0.0 && cd ../..
 
-                go test -mod=vendor -v ./contrib/google.golang.org/grpc.v12/...
+                INTEGRATION=true go test -mod=vendor -v ./contrib/google.golang.org/grpc.v12/...
 
       - save_cache:
           key: go-mod-v5-contrib-{{ checksum "go.sum.orig" }}

.github/workflows/system-tests.yml

@@ -0,0 +1,53 @@
+name: System Tests
+
+on:
+  pull_request:
+    branches:
+      - "**"
+  workflow_dispatch: {}
+  schedule:
+    - cron:  '00 04 * * 2-6'
+
+jobs:
+  system-tests:
+    if: ${{ github.event.pull_request.head.repo.full_name == 'DataDog/dd-trace-go' }}
+    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        include:
+          - library: golang
+            weblog-variant: net-http
+          - library: golang
+            weblog-variant: gorilla
+      fail-fast: false
+    env:
+      TEST_LIBRARY: golang
+      WEBLOG_VARIANT: ${{ matrix.weblog-variant }}
+      DD_API_KEY: ${{ secrets.DD_API_KEY }}
+    steps:
+      - name: Checkout system tests
+        uses: actions/checkout@v2
+        with:
+          repository: 'DataDog/system-tests'
+
+      - name: Checkout dd-trace-go
+        uses: actions/checkout@v2
+        with:
+          path: 'binaries/dd-trace-go'
+
+      - name: Build
+        run: ./build.sh
+
+      - name: Run
+        run: ./run.sh
+
+      - name: Compress artifact
+        if: ${{ always() }}
+        run: tar -czvf artifact.tar.gz $(ls | grep logs)
+
+      - name: Upload artifact
+        uses: actions/upload-artifact@v2
+        if: ${{ always() }}
+        with:
+          name: logs_${{ matrix.weblog-variant }}
+          path: artifact.tar.gz

.gitignore

@@ -12,6 +12,7 @@ bin/
 *.swp
 .idea
 dd-trace-go.iml
+vendor
 
 /contrib/google.golang.org/grpc.v12/vendor/
 

FAQ.md

@@ -0,0 +1,41 @@
+# Frequently Asked Questions (dd-trace-go)
+This document contains answers to questions frequently asked by users. Just because something is listed here doesn't mean it's beyond question, so feel free to open an issue if you want to change or improve any of these things, but this should provide useful information about *how* things are and *why* they are that way.
+
+#### Why does dd-trace-go use go modules in a non-standard way?
+This repository currently takes an [idiosyncratic approach](https://github.com/DataDog/dd-trace-go/issues/810) to using Go modules. You may notice that the `go.mod` file does not list all dependencies of the project, but instead just lists the ones that the core logic depends on (`ddtrace/...`, `profiler/...` and a couple other packages).
+
+As a user of the `dd-trace-go` module, this should be transparent and, if anything, make life easier for you. For those wanting to contribute, this can be confusing and annoying.
+
+The primary reason that we do this is for compatibility. `dd-trace-go` contains many contrib packages for many many libraries, all of which are optional features. If we included all of the dd-trace-go dependencies in the `go.mod` file, users' projects would transitively include dependencies for multiple web frameworks, grpc, sql libraries, redis, and lots of other stuff. That's bothersome enough, but worse is that users may end up having versions of libraries forcibly updated, even when they're not using integrations for those libraries. If a user wanted to include `dd-trace-go` to trace their web router but was using an old version of `go-redis` or somesuch, they would be forced to update, even if they don't want to trace `go-redis` and `dd-trace-go` doesn't actually need it. The more integrations we include in `dd-trace-go`, the more likely this situation becomes when we add all dependencies into the `go.mod` file.
+
+Another reason is that we can't. Due to some of the projects we integrate with breaking semantic versioning, it's not possible for us to include all the required dependencies and versions, since we need to depend on multiple, incompatible minor versions of some packages, namely google.golang.org/grpc (See: https://github.com/grpc/grpc-go/issues/3726)
+
+The way forward is through submodules, allowing us to have multiple dependency sets inside `dd-trace-go` and only including what's actually necessary in a user's application. Unfortunately, a prerequisite for this is to get away from `gopkg.in`, which does not support submodules.
+
+Please follow: [848](https://github.com/DataDog/dd-trace-go/issues/848) and [922](https://github.com/DataDog/dd-trace-go/pull/922)
+
+Note also that the [Contribution Guidelines](https://github.com/DataDog/dd-trace-go/blob/v1/CONTRIBUTING.md#go-modules) contain information on working with this setup.
+
+
+#### Why do client integration spans not use the global service name?
+Integrations that are considered *clients* (http clients, grpc clients, sql clients) do **not** use the globally-configured service name. This is by design and is a product-level decision that spans across all the languages' tracers. This is likely to segregate the time spent actually doing the work of the service from the time waiting for another service (i.e. waiting on a web server to return a response).
+
+While there are good arguments to be made that client integrations should take the same service name as everything else in the service, that's not how the library is intended to function today. As a work-around, most integrations have a `WithServiceName` `Option` that will allow you to override the default. If the integration you are using cannot be configured the way you want, please open an issue to discuss adding as option.
+
+#### Why are client integration spans not measured?
+This is primarily for 2 reasons:
+1. Cost - often a traced client will speak to a traced server. If both are measured, there is duplication of measurement here, and duplication of cost for no benefit. By measuring **only** the server, we get analytics without duplication. 
+2. Name conflicts - Today, metrics are calculated based on a key of the span's service name and operation name. This can cause clashes when a client and server both use the same operation name.
+
+For example, `net/http` [server tracing](https://github.com/DataDog/dd-trace-go/blob/f86a82b0ae679be3bbd2fe3652ae17f06aabd960/contrib/internal/httputil/trace.go#L52):
+```
+span, ctx := tracer.StartSpanFromContext(cfg.Request.Context(), "http.request", opts...)
+```
+
+and `net/http` [client tracing](https://github.com/DataDog/dd-trace-go/blob/f86a82b0ae679be3bbd2fe3652ae17f06aabd960/contrib/net/http/roundtripper.go#L39):
+```
+span, ctx := tracer.StartSpanFromContext(req.Context(), "http.request", opts...)
+```
+
+This is something that users ask for from time to time, and there is work internally to resolve this. Please follow [#1006](https://github.com/DataDog/dd-trace-go/issues/1006) to track the progress.
+

checkcopyright.go

@@ -3,6 +3,7 @@
 // This product includes software developed at Datadog (https://www.datadoghq.com/).
 // Copyright 2016 Datadog, Inc.
 
+//go:build ignore
 // +build ignore
 
 // This tool validates that all *.go files in the repository have the copyright text attached.
@@ -22,6 +23,8 @@ func main() {
 	// 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.`)
+	generatedRegexp := regexp.MustCompile(`Code generated by.+DO NOT EDIT`)
+
 	if err := filepath.Walk(".", func(path string, info os.FileInfo, err error) error {
 		if err != nil {
 			return err
@@ -40,7 +43,7 @@ func main() {
 		if err != nil && err != io.EOF {
 			return err
 		}
-		if !copyrightRegexp.Match(snip) {
+		if !copyrightRegexp.Match(snip) && !generatedRegexp.Match(snip) {
 			// report missing header
 			missing = true
 			log.Printf("Copyright header missing in %q.\n", path)

codecov.yml

@@ -15,6 +15,11 @@ coverage:
     patch:
       default:
         target: 90%
+        # Only run this check for pull requests, but skip it for merged
+        # commits. This highlights patches with potential coverage problems
+        # during development, but doesn't make our v1 (release) branch look
+        # like it fails CI checks as we consider this check to be optional.
+        only_pulls: true
 
 flags:
   core:

contrib/database/sql/conn_test.go

@@ -77,6 +77,7 @@ func TestWithSpanTags(t *testing.T) {
 	for _, tt := range testcases {
 		t.Run(tt.name, func(t *testing.T) {
 			Register(tt.sqlRegister.name, tt.sqlRegister.driver, tt.sqlRegister.opts...)
+			defer unregister(tt.sqlRegister.name)
 			db, err := Open(tt.sqlRegister.name, tt.sqlRegister.dsn)
 			if err != nil {
 				log.Fatal(err)
@@ -143,6 +144,7 @@ func TestWithChildSpansOnly(t *testing.T) {
 	for _, tt := range testcases {
 		t.Run(tt.name, func(t *testing.T) {
 			Register(tt.sqlRegister.name, tt.sqlRegister.driver, tt.sqlRegister.opts...)
+			defer unregister(tt.sqlRegister.name)
 			db, err := Open(tt.sqlRegister.name, tt.sqlRegister.dsn)
 			require.NoError(t, err)
 			defer db.Close()

contrib/database/sql/sql.go

@@ -79,6 +79,14 @@ func (d *driverRegistry) config(name string) (*config, bool) {
 	return config, ok
 }
 
+// unregister is used to make tests idempotent.
+func (d *driverRegistry) unregister(name string) {
+	driver := d.drivers[name]
+	delete(d.keys, reflect.TypeOf(driver))
+	delete(d.configs, name)
+	delete(d.drivers, name)
+}
+
 // Register tells the sql integration package about the driver that we will be tracing. It must
 // be called before Open, if that connection is to be traced. It uses the driverName suffixed
 // with ".db" as the default service name.
@@ -103,6 +111,13 @@ func Register(driverName string, driver driver.Driver, opts ...RegisterOption) {
 	registeredDrivers.add(driverName, driver, cfg)
 }
 
+// unregister is used to make tests idempotent.
+func unregister(name string) {
+	if registeredDrivers.isRegistered(name) {
+		registeredDrivers.unregister(name)
+	}
+}
+
 // errNotRegistered is returned when there is an attempt to open a database connection towards a driver
 // that has not previously been registered using this package.
 var errNotRegistered = errors.New("sqltrace: Register must be called before Open")

contrib/elastic/go-elasticsearch.v6/elastictrace_test.go

@@ -37,7 +37,7 @@ func TestMain(m *testing.M) {
 }
 
 func checkPUTTrace(assert *assert.Assertions, mt mocktracer.Tracer) {
-	span := mt.FinishedSpans()[0]
+	span := mt.FinishedSpans()[1]
 	assert.Equal("my-es-service", span.Tag(ext.ServiceName))
 	assert.Equal("PUT /twitter/tweet/?", span.Tag(ext.ResourceName))
 	assert.Equal("/twitter/tweet/1", span.Tag("elasticsearch.url"))

contrib/elastic/go-elasticsearch.v6/elastictrace_v7_test.go

@@ -86,7 +86,7 @@ func TestClientErrorCutoffV7(t *testing.T) {
 	}.Do(context.Background(), client)
 	assert.NoError(err)
 
-	span := mt.FinishedSpans()[0]
+	span := mt.FinishedSpans()[1]
 	assert.Equal(`{"error":{`, span.Tag(ext.Error).(error).Error())
 }
 
@@ -113,8 +113,6 @@ func TestClientV7Failure(t *testing.T) {
 	assert.Error(err)
 
 	spans := mt.FinishedSpans()
-	checkPUTTrace(assert, mt)
-
 	assert.NotEmpty(spans[0].Tag(ext.Error))
 	assert.Equal("*net.OpError", fmt.Sprintf("%T", spans[0].Tag(ext.Error).(error)))
 }
@@ -144,7 +142,7 @@ func TestResourceNamerSettingsV7(t *testing.T) {
 			DocumentType: "tweet",
 		}.Do(context.Background(), client)
 
-		span := mt.FinishedSpans()[0]
+		span := mt.FinishedSpans()[1]
 		assert.Equal(t, "GET /logs_?_?/event/_search/tweet/?", span.Tag(ext.ResourceName))
 	})
 
@@ -193,8 +191,8 @@ func TestAnalyticsSettingsV7(t *testing.T) {
 		assert.NoError(t, err)
 
 		spans := mt.FinishedSpans()
-		assert.Len(t, spans, 1)
-		s := spans[0]
+		assert.Len(t, spans, 2)
+		s := spans[1]
 		assert.Equal(t, rate, s.Tag(ext.EventSampleRate))
 	}
 

contrib/gin-gonic/gin/gintrace.go

@@ -29,6 +29,9 @@ func Middleware(service string, opts ...Option) gin.HandlerFunc {
 	}
 	log.Debug("contrib/gin-gonic/gin: Configuring Middleware: Service: %s, %#v", service, cfg)
 	return func(c *gin.Context) {
+		if cfg.ignoreRequest(c) {
+			return
+		}
 		resource := cfg.resourceNamer(c)
 		opts := []ddtrace.StartSpanOption{
 			tracer.ServiceName(cfg.serviceName),

contrib/gin-gonic/gin/gintrace_test.go

@@ -10,6 +10,7 @@ import (
 	"fmt"
 	"html/template"
 	"net/http/httptest"
+	"strings"
 	"testing"
 
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/ext"
@@ -340,6 +341,34 @@ func TestResourceNamerSettings(t *testing.T) {
 	})
 }
 
+func TestIgnoreRequestSettings(t *testing.T) {
+	router := gin.New()
+	router.Use(Middleware("foobar", WithIgnoreRequest(func(c *gin.Context) bool {
+		return strings.HasPrefix(c.Request.URL.Path, "/skip")
+	})))
+
+	router.GET("/OK", func(c *gin.Context) {
+		c.Writer.Write([]byte("OK"))
+	})
+
+	router.GET("/skip", func(c *gin.Context) {
+		c.Writer.Write([]byte("Skip"))
+	})
+
+	for path, shouldSkip := range map[string]bool{
+		"/OK":      false,
+		"/skip":    true,
+		"/skipfoo": true,
+	} {
+		mt := mocktracer.Start()
+		defer mt.Reset()
+
+		r := httptest.NewRequest("GET", "http://localhost"+path, nil)
+		router.ServeHTTP(httptest.NewRecorder(), r)
+		assert.Equal(t, shouldSkip, len(mt.FinishedSpans()) == 0)
+	}
+}
+
 func TestServiceName(t *testing.T) {
 	t.Run("default", func(t *testing.T) {
 		assert := assert.New(t)

contrib/gin-gonic/gin/option.go

@@ -19,6 +19,7 @@ type config struct {
 	analyticsRate float64
 	resourceNamer func(c *gin.Context) string
 	serviceName   string
+	ignoreRequest func(c *gin.Context) bool
 }
 
 func newConfig(service string) *config {
@@ -36,6 +37,7 @@ func newConfig(service string) *config {
 		analyticsRate: rate,
 		resourceNamer: defaultResourceNamer,
 		serviceName:   service,
+		ignoreRequest: func(_ *gin.Context) bool { return false },
 	}
 }
 
@@ -73,6 +75,14 @@ func WithResourceNamer(namer func(c *gin.Context) string) Option {
 	}
 }
 
+// WithIgnoreRequest specifies a function to use for determining if the
+// incoming HTTP request tracing should be skipped.
+func WithIgnoreRequest(f func(c *gin.Context) bool) Option {
+	return func(cfg *config) {
+		cfg.ignoreRequest = f
+	}
+}
+
 func defaultResourceNamer(c *gin.Context) string {
 	// getName is a hacky way to check whether *gin.Context implements the FullPath()
 	// method introduced in v1.4.0, falling back to the previous implementation otherwise.