diff --git a/.golangci.yaml b/.golangci.yaml new file mode 100644 index 00000000..b94492a6 --- /dev/null +++ b/.golangci.yaml @@ -0,0 +1,35 @@ +run: + deadline: 3m + +linters: + disable-all: true + enable: + - deadcode + - depguard + - dupl + - errcheck + - gochecknoinits + - gocritic + - gocyclo + - gofmt + - goimports + - golint + - gosec + - gosimple + - govet + - ineffassign + - interfacer + - maligned + - megacheck + - misspell + - nakedret + - prealloc + - scopelint + - staticcheck + - structcheck + - stylecheck + - typecheck + - unconvert + - unparam + - unused + - varcheck diff --git a/.travis.yml b/.travis.yml index b0f54c92..3ceb6bcb 100644 --- a/.travis.yml +++ b/.travis.yml @@ -7,7 +7,7 @@ sudo: required go: - "1.9" - "1.10" - - "1.11" + - "1.11" - "1.12" - "tip" @@ -41,9 +41,18 @@ install: else go get; fi + # Only bother running lints on the latest version. Some of the linters fail to install on older versions of go. + - if [ $TRAVIS_GO_VERSION == "1.12" ]; then + go get github.com/golangci/golangci-lint/cmd/golangci-lint@v1.16.0; + fi script: - make test-with-race + # We are in the process of resolving all the lint warnings for x-ray-sdk-go. Still run the linter to expose these + # and resolve to clean these up. + - if [ $TRAVIS_GO_VERSION == "1.12" ]; then + make golangci-lint || true; + fi matrix: allow_failures: diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a622118..638e802b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,21 @@ +Release v1.0.0-rc.12 (2019-06-11) +================================ +### SDK Breaking Changes +* Updates `sampling.NewProxy` method to be private. [PR #93](https://github.com/aws/aws-xray-sdk-go/pull/93) + +### SDK Enhancements +* Fixes a bug for failing to close in-progress `connect` subsegments in some cases. [PR #102](https://github.com/aws/aws-xray-sdk-go/pull/102) +* Fixes data races condition. [PR #103](https://github.com/aws/aws-xray-sdk-go/pull/103) +* Fixes a nil pointer issue. [PR #109](https://github.com/aws/aws-xray-sdk-go/pull/109) +* Refactors `newGlobalConfig` to avoid initializing log. [PR #96](https://github.com/aws/aws-xray-sdk-go/pull/96) +* Adds `-race` to travis test script. [PR #104](https://github.com/aws/aws-xray-sdk-go/pull/104) +* Fixes data race condition for parallel http client request. [PR #100](https://github.com/aws/aws-xray-sdk-go/pull/100) +* Adds support for `tx.Prepare`. [PR #95](https://github.com/aws/aws-xray-sdk-go/pull/95) +* Fixes race bugs with `ClientTrace`. [PR #115](https://github.com/aws/aws-xray-sdk-go/pull/115) +* Updates lock abstraction for `defaultLogger`. [PR #113](https://github.com/aws/aws-xray-sdk-go/pull/113) +* Adds `golangci-lint` into travis CI. [PR #114](https://github.com/aws/aws-xray-sdk-go/pull/114) +* Fixes uncaught error on SQL url parse. [PR #121](https://github.com/aws/aws-xray-sdk-go/pull/121) + Release v1.0.0-rc.11 (2019-03-15) ================================ ### SDK Breaking Changes diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index df32f379..afbc4687 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -3,6 +3,7 @@ The following people have contributed to the AWS X-Ray SDK for Go's design and/o * Anssi Alaranta * Bilal Khan * Christopher Radek +* Jacob Rickerd * James Bowman * Lulu Zhao * Muir Manders diff --git a/go.mod b/go.mod index 4b0e5ce8..eb9a2aca 100644 --- a/go.mod +++ b/go.mod @@ -3,11 +3,9 @@ module github.com/aws/aws-xray-sdk-go require ( github.com/DATA-DOG/go-sqlmock v1.2.0 github.com/aws/aws-sdk-go v1.17.12 - github.com/davecgh/go-spew v0.0.0-20160907170601-6d212800a42e - github.com/jmespath/go-jmespath v0.0.0-20180206201540-c2b33e8439af + github.com/davecgh/go-spew v0.0.0-20160907170601-6d212800a42e // indirect github.com/pkg/errors v0.8.1 - github.com/pmezard/go-difflib v0.0.0-20151028094244-d8ed2627bdf0 + github.com/pmezard/go-difflib v0.0.0-20151028094244-d8ed2627bdf0 // indirect github.com/stretchr/testify v1.1.4 - golang.org/x/net v0.0.0-20190301231341-16b79f2e4e95 - golang.org/x/text v0.0.0-20190306152657-5d731a35f486 + golang.org/x/net v0.0.0-20190311183353-d8887717615a ) diff --git a/go.sum b/go.sum index 0be3bef1..b23bbb6b 100644 --- a/go.sum +++ b/go.sum @@ -12,8 +12,9 @@ github.com/pmezard/go-difflib v0.0.0-20151028094244-d8ed2627bdf0 h1:GD+A8+e+wFkq github.com/pmezard/go-difflib v0.0.0-20151028094244-d8ed2627bdf0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/stretchr/testify v1.1.4 h1:ToftOQTytwshuOSj6bDSolVUa3GINfJP/fg3OkkOzQQ= github.com/stretchr/testify v1.1.4/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= -golang.org/x/net v0.0.0-20190301231341-16b79f2e4e95 h1:fY7Dsw114eJN4boqzVSbpVHO6rTdhq6/GnXeu+PKnzU= -golang.org/x/net v0.0.0-20190301231341-16b79f2e4e95/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= -golang.org/x/text v0.0.0-20190306152657-5d731a35f486 h1:XzEEnDs8NtiSU8gJvzuDjv+Kam+0nPN9pjOA3oZlxIY= -golang.org/x/text v0.0.0-20190306152657-5d731a35f486/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk= -golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= \ No newline at end of file +golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= +golang.org/x/net v0.0.0-20190311183353-d8887717615a h1:oWX7TPOiFAMXLq8o0ikBYfCJVlRHBcsciT5bXOrH628= +golang.org/x/net v0.0.0-20190311183353-d8887717615a/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= +golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= +golang.org/x/text v0.3.0 h1:g61tztE5qeGQ89tm6NTjjM9VPIm088od1l6aSorWRWg= +golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= diff --git a/internal/logger/logger.go b/internal/logger/logger.go index b3c99a90..6ab83266 100644 --- a/internal/logger/logger.go +++ b/internal/logger/logger.go @@ -15,7 +15,7 @@ import ( "github.com/aws/aws-xray-sdk-go/xraylog" ) -// This internal pacakge hides the actual logging functions from the user. +// This internal package hides the actual logging functions from the user. // The Logger instance used by xray to log. Set via xray.SetLogger(). var Logger xraylog.Logger = xraylog.NewDefaultLogger(os.Stdout, xraylog.LogLevelInfo) diff --git a/makefile b/makefile index eb6232fb..483e654f 100644 --- a/makefile +++ b/makefile @@ -19,3 +19,6 @@ test-with-race: test fmt: go fmt `go list ./... | grep -v vendor` + +golangci-lint: + golangci-lint run diff --git a/resources/bindata.go b/resources/bindata.go index 22d05656..907704da 100644 --- a/resources/bindata.go +++ b/resources/bindata.go @@ -182,7 +182,7 @@ func AssetNames() []string { // _bindata is a table, holding each asset generator, mapped to its name. var _bindata = map[string]func() (*asset, error){ - "resources/AWSWhitelist.json": resourcesAwswhitelistJson, + "resources/AWSWhitelist.json": resourcesAwswhitelistJson, "resources/DefaultSamplingRules.json": resourcesDefaultsamplingrulesJson, "resources/ExampleSamplingRules.json": resourcesExamplesamplingrulesJson, } @@ -226,9 +226,10 @@ type bintree struct { Func func() (*asset, error) Children map[string]*bintree } + var _bintree = &bintree{nil, map[string]*bintree{ "resources": &bintree{nil, map[string]*bintree{ - "AWSWhitelist.json": &bintree{resourcesAwswhitelistJson, map[string]*bintree{}}, + "AWSWhitelist.json": &bintree{resourcesAwswhitelistJson, map[string]*bintree{}}, "DefaultSamplingRules.json": &bintree{resourcesDefaultsamplingrulesJson, map[string]*bintree{}}, "ExampleSamplingRules.json": &bintree{resourcesExamplesamplingrulesJson, map[string]*bintree{}}, }}, @@ -280,4 +281,3 @@ func _filePath(dir, name string) string { cannonicalName := strings.Replace(name, "\\", "/", -1) return filepath.Join(append([]string{dir}, strings.Split(cannonicalName, "/")...)...) } - diff --git a/strategy/sampling/centralized.go b/strategy/sampling/centralized.go index 01053b0f..a0426ecf 100644 --- a/strategy/sampling/centralized.go +++ b/strategy/sampling/centralized.go @@ -433,7 +433,7 @@ func (ss *CentralizedStrategy) refreshTargets() (err error) { logger.Infof("Refreshing sampling rules out-of-band.") go func() { - if err = ss.refreshManifest(); err != nil { + if err := ss.refreshManifest(); err != nil { logger.Debugf("Error occurred refreshing sampling rules out-of-band. %v", err) } }() diff --git a/strategy/sampling/centralized_test.go b/strategy/sampling/centralized_test.go index 6639b534..277735ab 100644 --- a/strategy/sampling/centralized_test.go +++ b/strategy/sampling/centralized_test.go @@ -966,7 +966,7 @@ func TestRefreshManifestRuleAddition(t *testing.T) { assert.Equal(t, r2, ss.manifest.Rules[1]) assert.Equal(t, r3, ss.manifest.Rules[2]) - // Assert on size of manfiest + // Assert on size of manifest assert.Equal(t, 3, len(ss.manifest.Rules)) assert.Equal(t, 3, len(ss.manifest.Index)) diff --git a/xray/aws.go b/xray/aws.go index 22baf7fe..1943aa6c 100644 --- a/xray/aws.go +++ b/xray/aws.go @@ -88,19 +88,6 @@ var xRayBeforeSignHandler = request.NamedHandler{ }, } -var xRayAfterSignHandler = request.NamedHandler{ - Name: "XRayAfterSignHandler", - Fn: func(r *request.Request) { - endSubsegment(r) - }, -} - -var xRayBeforeSendHandler = request.NamedHandler{ - Name: "XRayBeforeSendHandler", - Fn: func(r *request.Request) { - }, -} - var xRayAfterSendHandler = request.NamedHandler{ Name: "XRayAfterSendHandler", Fn: func(r *request.Request) { @@ -299,8 +286,7 @@ func (j *jsonMap) data() interface{} { } func (j *jsonMap) search(keys ...string) *jsonMap { - var object interface{} - object = j.data() + object := j.data() for target := 0; target < len(keys); target++ { if mmap, ok := object.(map[string]interface{}); ok { diff --git a/xray/config.go b/xray/config.go index db152a63..717df81f 100644 --- a/xray/config.go +++ b/xray/config.go @@ -24,7 +24,7 @@ import ( ) // SDKVersion records the current X-Ray Go SDK version. -const SDKVersion = "1.0.0-rc.11" +const SDKVersion = "1.0.0-rc.12" // SDKType records which X-Ray SDK customer uses. const SDKType = "X-Ray for Go" diff --git a/xray/config_test.go b/xray/config_test.go index 0376dde1..a2c2ef7d 100644 --- a/xray/config_test.go +++ b/xray/config_test.go @@ -72,7 +72,7 @@ func (te *TestEmitter) Emit(seg *Segment) {} func (te *TestEmitter) RefreshEmitterWithAddress(raddr *net.UDPAddr) {} func (cms *TestContextMissingStrategy) ContextMissing(v interface{}) { - fmt.Sprintf("Test ContextMissing Strategy %v", v) + fmt.Printf("Test ContextMissing Strategy %v\n", v) } func stashEnv() []string { diff --git a/xray/default_emitter_test.go b/xray/default_emitter_test.go index 1f238c64..a6ca71a6 100644 --- a/xray/default_emitter_test.go +++ b/xray/default_emitter_test.go @@ -11,10 +11,11 @@ package xray import ( "encoding/json" "fmt" - "github.com/stretchr/testify/assert" "math/rand" "testing" "time" + + "github.com/stretchr/testify/assert" ) func TestNoNeedStreamingStrategy(t *testing.T) { diff --git a/xray/default_streaming_strategy_test.go b/xray/default_streaming_strategy_test.go index 88a9c138..dadd5d05 100644 --- a/xray/default_streaming_strategy_test.go +++ b/xray/default_streaming_strategy_test.go @@ -9,8 +9,9 @@ package xray import ( - "github.com/stretchr/testify/assert" "testing" + + "github.com/stretchr/testify/assert" ) func TestDefaultStreamingStrategyMaxSegmentSize(t *testing.T) { diff --git a/xray/httptrace.go b/xray/httptrace.go index c56c40fa..dc6fb2ab 100644 --- a/xray/httptrace.go +++ b/xray/httptrace.go @@ -19,6 +19,11 @@ import ( ) // HTTPSubsegments is a set of context in different HTTP operation. +// Note: from ClientTrace godoc +// Functions may be called concurrently from different goroutines +// +// HTTPSubsegments must operate as though all functions on it can be called in +// different goroutines and must protect against races type HTTPSubsegments struct { opCtx context.Context connCtx context.Context @@ -39,6 +44,8 @@ func NewHTTPSubsegments(opCtx context.Context) *HTTPSubsegments { // GetConn begins a connect subsegment if the HTTP operation // subsegment is still in progress. func (xt *HTTPSubsegments) GetConn(hostPort string) { + xt.mu.Lock() + defer xt.mu.Unlock() if GetSegment(xt.opCtx).safeInProgress() { xt.connCtx, _ = BeginSubsegment(xt.opCtx, "connect") } @@ -60,6 +67,8 @@ func (xt *HTTPSubsegments) DNSStart(info httptrace.DNSStartInfo) { // and whether or not the call was coalesced is added as // metadata to the dns subsegment. func (xt *HTTPSubsegments) DNSDone(info httptrace.DNSDoneInfo) { + xt.mu.Lock() + defer xt.mu.Unlock() if xt.dnsCtx != nil && GetSegment(xt.opCtx).safeInProgress() { metadata := make(map[string]interface{}) metadata["addresses"] = info.Addrs @@ -85,6 +94,8 @@ func (xt *HTTPSubsegments) ConnectStart(network, addr string) { // (if any). Information about the network over which the dial // was made is added as metadata to the subsegment. func (xt *HTTPSubsegments) ConnectDone(network, addr string, err error) { + xt.mu.Lock() + defer xt.mu.Unlock() if xt.connectCtx != nil && GetSegment(xt.opCtx).safeInProgress() { metadata := make(map[string]interface{}) metadata["network"] = network @@ -97,6 +108,8 @@ func (xt *HTTPSubsegments) ConnectDone(network, addr string, err error) { // TLSHandshakeStart begins a tls subsegment if the HTTP operation // subsegment is still in progress. func (xt *HTTPSubsegments) TLSHandshakeStart() { + xt.mu.Lock() + defer xt.mu.Unlock() if GetSegment(xt.opCtx).safeInProgress() && xt.connCtx != nil { xt.tlsCtx, _ = BeginSubsegment(xt.connCtx, "tls") } @@ -107,6 +120,8 @@ func (xt *HTTPSubsegments) TLSHandshakeStart() { // error value(if any). Information about the tls connection // is added as metadata to the subsegment. func (xt *HTTPSubsegments) TLSHandshakeDone(connState tls.ConnectionState, err error) { + xt.mu.Lock() + defer xt.mu.Unlock() if xt.tlsCtx != nil && GetSegment(xt.opCtx).safeInProgress() { metadata := make(map[string]interface{}) metadata["did_resume"] = connState.DidResume @@ -125,14 +140,14 @@ func (xt *HTTPSubsegments) TLSHandshakeDone(connState tls.ConnectionState, err e // metadata to the subsegment. If the connection is marked as reused, // the connect subsegment is deleted. func (xt *HTTPSubsegments) GotConn(info *httptrace.GotConnInfo, err error) { + xt.mu.Lock() + defer xt.mu.Unlock() if xt.connCtx != nil && GetSegment(xt.opCtx).safeInProgress() { // GetConn may not have been called (client_test.TestBadRoundTrip) if info != nil { if info.Reused { GetSegment(xt.opCtx).RemoveSubsegment(GetSegment(xt.connCtx)) - xt.mu.Lock() // Remove the connCtx context since it is no longer needed. xt.connCtx = nil - xt.mu.Unlock() } else { metadata := make(map[string]interface{}) metadata["reused"] = info.Reused @@ -159,12 +174,12 @@ func (xt *HTTPSubsegments) GotConn(info *httptrace.GotConnInfo, err error) { // subsegment is still in progress, passing the error value // (if any). The response subsegment is then begun. func (xt *HTTPSubsegments) WroteRequest(info httptrace.WroteRequestInfo) { + xt.mu.Lock() + defer xt.mu.Unlock() if xt.reqCtx != nil && GetSegment(xt.opCtx).InProgress { GetSegment(xt.reqCtx).Close(info.Err) resCtx, _ := BeginSubsegment(xt.opCtx, "response") - xt.mu.Lock() xt.responseCtx = resCtx - xt.mu.Unlock() } // In case the GotConn http trace handler wasn't called, @@ -180,8 +195,8 @@ func (xt *HTTPSubsegments) WroteRequest(info httptrace.WroteRequestInfo) { // operation subsegment is still in progress. func (xt *HTTPSubsegments) GotFirstResponseByte() { xt.mu.Lock() + defer xt.mu.Unlock() resCtx := xt.responseCtx - xt.mu.Unlock() if resCtx != nil && GetSegment(xt.opCtx).InProgress { GetSegment(resCtx).Close(nil) } diff --git a/xray/lambda_test.go b/xray/lambda_test.go index 5e1e5bd7..5afc84c6 100644 --- a/xray/lambda_test.go +++ b/xray/lambda_test.go @@ -2,8 +2,9 @@ package xray import ( "context" - "github.com/stretchr/testify/assert" "testing" + + "github.com/stretchr/testify/assert" ) func TestLambdaSegmentEmit(t *testing.T) { diff --git a/xray/segment.go b/xray/segment.go index 0f358e65..75912236 100644 --- a/xray/segment.go +++ b/xray/segment.go @@ -162,7 +162,7 @@ func BeginSubsegment(ctx context.Context, name string) (context.Context, *Segmen name = name[:200] } - parent := &Segment{} + var parent *Segment // first time to create facade segment if getTraceHeaderFromContext(ctx) != nil && GetSegment(ctx) == nil { _, parent = newFacadeSegment(ctx) @@ -445,10 +445,8 @@ func (seg *Segment) AddError(err error) error { return nil } -func (seg *Segment) addError(err error) error { +func (seg *Segment) addError(err error) { seg.Fault = true seg.GetCause().WorkingDirectory, _ = os.Getwd() seg.GetCause().Exceptions = append(seg.GetCause().Exceptions, seg.ParentSegment.GetConfiguration().ExceptionFormattingStrategy.ExceptionFromError(err)) - - return nil } diff --git a/xray/segment_test.go b/xray/segment_test.go index 760407c9..dc19b301 100644 --- a/xray/segment_test.go +++ b/xray/segment_test.go @@ -39,7 +39,7 @@ func TestSubsegmentDataRace(t *testing.T) { go func() { defer wg.Done() ctx, seg := BeginSubsegment(ctx, "TestSubsegment1") - ctx, seg2 := BeginSubsegment(ctx, "TestSubsegment2") + _, seg2 := BeginSubsegment(ctx, "TestSubsegment2") seg2.Close(nil) seg.Close(nil) }() diff --git a/xray/sql.go b/xray/sql.go index fb9afa58..83f8e396 100644 --- a/xray/sql.go +++ b/xray/sql.go @@ -42,7 +42,10 @@ func SQL(driver, dsn string) (*DB, error) { if u, err := url.Parse(urlDsn); err == nil && (u.Scheme != "" || u.User != nil || u.RawQuery != "" || strings.Contains(u.Path, "@")) { // Check that this isn't in the form of user/pass@host:port/db, as that will shove the host into the path if strings.Contains(u.Path, "@") { - u, _ = url.Parse(fmt.Sprintf("%s//%s%%2F%s", u.Scheme, u.Host, u.Path[1:])) + u, err = url.Parse(fmt.Sprintf("%s//%s%%2F%s", u.Scheme, u.Host, u.Path[1:])) + if err != nil { + return nil, err + } } // Strip password from user:password pair in address @@ -292,4 +295,4 @@ func processNilSegment(ctx context.Context) { } else { globalCfg.ContextMissingStrategy().ContextMissing(failedMessage) } -} \ No newline at end of file +} diff --git a/xray/sql_go18.go b/xray/sql_go18.go index 2ee0e30c..0067207a 100644 --- a/xray/sql_go18.go +++ b/xray/sql_go18.go @@ -82,6 +82,12 @@ func (db *DB) QueryRow(ctx context.Context, query string, args ...interface{}) * return res } +// Prepare creates a prepared statement for later queries or executions. +func (tx *Tx) Prepare(ctx context.Context, query string) (*Stmt, error) { + stmt, err := tx.tx.PrepareContext(ctx, query) + return &Stmt{tx.db, stmt, query}, err +} + // Exec captures executing a query that doesn't return rows and adds // corresponding information into subsegment. func (tx *Tx) Exec(ctx context.Context, query string, args ...interface{}) (sql.Result, error) { diff --git a/xray/util_test.go b/xray/util_test.go index 5b47ab3d..e85a2ecf 100644 --- a/xray/util_test.go +++ b/xray/util_test.go @@ -11,10 +11,11 @@ package xray import ( "context" "encoding/json" - "github.com/aws/aws-xray-sdk-go/header" "net" "net/http" "time" + + "github.com/aws/aws-xray-sdk-go/header" ) var ( @@ -74,7 +75,7 @@ func (td *Testdaemon) Run() { } func (td *Testdaemon) Recv() (*Segment, error) { - ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond) defer cancel() select { case r := <-td.Channel: diff --git a/xraylog/xray_log.go b/xraylog/xray_log.go index 5883bf91..4d9c439d 100644 --- a/xraylog/xray_log.go +++ b/xraylog/xray_log.go @@ -61,7 +61,7 @@ func NewDefaultLogger(w io.Writer, minLogLevel LogLevel) Logger { } type defaultLogger struct { - sync.Mutex + mu sync.Mutex w io.Writer minLevel LogLevel } @@ -71,8 +71,8 @@ func (l *defaultLogger) Log(ll LogLevel, msg fmt.Stringer) { return } - l.Lock() - defer l.Unlock() + l.mu.Lock() + defer l.mu.Unlock() fmt.Fprintf(l.w, "%s [%s] %s\n", time.Now().Format(time.RFC3339), ll, msg) }