diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 27c82d1..41ea01a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -30,4 +30,4 @@ jobs: go-version: stable - name: Run Unit Tests - run: go test -v ./... + run: go test ./... -race -shuffle=on -v diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 94ce172..b58a227 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -2,20 +2,21 @@ name: "CodeQL" on: push: - branches: [ "master" ] + branches: + - master pull_request: - branches: [ "master" ] + branches: + - master schedule: - cron: '41 15 * * 6' +concurrency: + group: ${{ github.ref_name }}-codeql + cancel-in-progress: true + jobs: analyze: name: Analyze (${{ matrix.language }}) - # Runner size impacts CodeQL analysis time. To learn more, please see: - # - https://gh.io/recommended-hardware-resources-for-running-codeql - # - https://gh.io/supported-runners-and-hardware-resources - # - https://gh.io/using-larger-runners (GitHub.com only) - # Consider using larger runners or machines with greater resources for possible analysis time improvements. runs-on: ubuntu-24.04 permissions: # required for all workflows diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml new file mode 100644 index 0000000..e1f9f9e --- /dev/null +++ b/.github/workflows/lint.yml @@ -0,0 +1,45 @@ +name: Lint + +on: + push: + branches: + - master + pull_request: + branches: + - master + +defaults: + run: + shell: bash + +concurrency: + group: ${{ github.ref_name }}-lint + cancel-in-progress: true + +jobs: + lint: + name: Go Lint + runs-on: ubuntu-24.04 + steps: + - name: Checkout Repository + uses: actions/checkout@v4 + + - name: Setup Golang Environment + uses: actions/setup-go@v5 + with: + go-version: stable + + - name: Lint Go + uses: golangci/golangci-lint-action@v6 + + actionlint: + name: Actionlint + runs-on: ubuntu-24.04 + steps: + - name: Checkout Repository + uses: actions/checkout@v4 + + - name: Lint Actions + uses: reviewdog/action-actionlint@v1 + with: + actionlint_flags: -shellcheck "" diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000..30c2b21 --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,91 @@ +linters-settings: + misspell: + locale: US + revive: + ignore-generated-header: true + rules: + - name: blank-imports + - name: context-as-argument + - name: context-keys-type + - name: dot-imports + - name: empty-block + - name: error-naming + - name: error-return + - name: error-strings + - name: errorf + - name: exported + - name: increment-decrement + - name: indent-error-flow + - name: package-comments + - name: range + - name: receiver-naming + - name: redefines-builtin-id + - name: superfluous-else + - name: time-naming + - name: unexported-return + - name: unreachable-code + - name: unused-parameter + - name: var-declaration + - name: var-naming + govet: + enable-all: true +linters: + enable: + - asasalint + - asciicheck + - bidichk + # - contextcheck + - copyloopvar + - dupword + - durationcheck + - errcheck + - errchkjson + - errname + - errorlint + - fatcontext + - forcetypeassert + - gocheckcompilerdirectives + - gochecksumtype + - gocritic + - godot + - gofmt + - gofumpt + - goimports + - gosec + - gosimple + - gosmopolitan + - govet + - ineffassign + - intrange + - makezero + - misspell + - musttag + - nilerr + - noctx + - nolintlint + - paralleltest + - perfsprint + - prealloc + - predeclared + - reassign + - revive + - staticcheck + - stylecheck + - tagalign + - tenv + - thelper + - tparallel + - typecheck + - unconvert + - unparam + - unused + - usestdlibvars + - wastedassign + - whitespace + # - wrapcheck + disable-all: true +issues: + max-issues-per-linter: 0 + max-same-issues: 0 +run: + timeout: 5m diff --git a/README.md b/README.md index e6b1065..b7b5e1e 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ systems in Go. ## Installation -``` +```shell go get github.com/opentracing-contrib/go-grpc ``` @@ -54,4 +54,3 @@ s := grpc.NewServer( // All future RPC activity involving `s` will be automatically traced. ``` - diff --git a/client.go b/client.go index c9496c8..2f7cdf7 100644 --- a/client.go +++ b/client.go @@ -1,12 +1,13 @@ package otgrpc import ( + "context" + "errors" "io" "runtime" "sync/atomic" opentracing "github.com/opentracing/opentracing-go" - "context" "github.com/opentracing/opentracing-go/ext" "github.com/opentracing/opentracing-go/log" "google.golang.org/grpc" @@ -18,10 +19,10 @@ import ( // // For example: // -// conn, err := grpc.Dial( -// address, -// ..., // (existing DialOptions) -// grpc.WithUnaryInterceptor(otgrpc.OpenTracingClientInterceptor(tracer))) +// conn, err := grpc.Dial( +// address, +// ..., // (existing DialOptions) +// grpc.WithUnaryInterceptor(otgrpc.OpenTracingClientInterceptor(tracer))) // // All gRPC client spans will inject the OpenTracing SpanContext into the gRPC // metadata; they will also look in the context.Context for an active @@ -80,10 +81,10 @@ func OpenTracingClientInterceptor(tracer opentracing.Tracer, optFuncs ...Option) // // For example: // -// conn, err := grpc.Dial( -// address, -// ..., // (existing DialOptions) -// grpc.WithStreamInterceptor(otgrpc.OpenTracingStreamClientInterceptor(tracer))) +// conn, err := grpc.Dial( +// address, +// ..., // (existing DialOptions) +// grpc.WithStreamInterceptor(otgrpc.OpenTracingStreamClientInterceptor(tracer))) // // All gRPC client spans will inject the OpenTracing SpanContext into the gRPC // metadata; they will also look in the context.Context for an active @@ -206,7 +207,7 @@ func (cs *openTracingClientStream) SendMsg(m interface{}) error { func (cs *openTracingClientStream) RecvMsg(m interface{}) error { err := cs.ClientStream.RecvMsg(m) - if err == io.EOF { + if errors.Is(err, io.EOF) { cs.finishFunc(nil) return err } else if err != nil { diff --git a/errors.go b/errors.go index 2f202af..b8212b2 100644 --- a/errors.go +++ b/errors.go @@ -21,7 +21,7 @@ const ( ServerError Class = "5xx" ) -// ErrorClass returns the class of the given error +// ErrorClass returns the class of the given error. func ErrorClass(err error) Class { if s, ok := status.FromError(err); ok { switch s.Code() { diff --git a/errors_test.go b/errors_test.go index 4c45651..ac173e5 100644 --- a/errors_test.go +++ b/errors_test.go @@ -16,6 +16,7 @@ const ( ) func TestSpanTags(t *testing.T) { + t.Parallel() tracer := mocktracer.New() for code := firstCode; code <= lastCode; code++ { // Client error diff --git a/go.mod b/go.mod index 95cb902..273cb42 100644 --- a/go.mod +++ b/go.mod @@ -4,7 +4,6 @@ go 1.21.0 require ( github.com/golang/protobuf v1.5.4 - github.com/grpc-ecosystem/grpc-opentracing v0.0.0-20180507213350-8e809c8a8645 github.com/opentracing/opentracing-go v1.2.0 github.com/stretchr/testify v1.9.0 google.golang.org/grpc v1.67.1 diff --git a/go.sum b/go.sum index dca590e..1d2aef9 100644 --- a/go.sum +++ b/go.sum @@ -5,8 +5,6 @@ github.com/golang/protobuf v1.5.4 h1:i7eJL8qZTpSEXOPTxNKhASYpMn+8e5Q6AdndVa1dWek github.com/golang/protobuf v1.5.4/go.mod h1:lnTiLA8Wa4RWRcIUkrtSVa5nRhsEGBg48fD6rSs7xps= github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= -github.com/grpc-ecosystem/grpc-opentracing v0.0.0-20180507213350-8e809c8a8645 h1:MJG/KsmcqMwFAkh8mTnAwhyKoB+sTAnY4CACC110tbU= -github.com/grpc-ecosystem/grpc-opentracing v0.0.0-20180507213350-8e809c8a8645/go.mod h1:6iZfnjpejD4L/4DwD7NryNaJyCQdzwWwH2MWhCA90Kw= github.com/opentracing/opentracing-go v1.2.0 h1:uEJPy/1a5RIPAJ0Ov+OIO8OxWu77jEv+1B0VhjKrZUs= github.com/opentracing/opentracing-go v1.2.0/go.mod h1:GxEUsuufX4nBwe+T+Wl9TAgYrxe9dPLANfrWvHYVTgc= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= diff --git a/options.go b/options.go index 5fac41e..1fdbe24 100644 --- a/options.go +++ b/options.go @@ -32,7 +32,7 @@ type SpanInclusionFunc func( method string, req, resp interface{}) bool -// IncludingSpans binds a IncludeSpanFunc to the options +// IncludingSpans binds a IncludeSpanFunc to the options. func IncludingSpans(inclusionFunc SpanInclusionFunc) Option { return func(o *options) { o.inclusionFunc = inclusionFunc @@ -60,10 +60,9 @@ func SpanDecorator(decorator SpanDecoratorFunc) Option { // scale well as production use dictates other configuration and tuning // parameters. type options struct { - logPayloads bool - decorator SpanDecoratorFunc - // May be nil. + decorator SpanDecoratorFunc inclusionFunc SpanInclusionFunc + logPayloads bool } // newOptions returns the default options. diff --git a/server.go b/server.go index c79d511..71f7e1a 100644 --- a/server.go +++ b/server.go @@ -1,8 +1,9 @@ package otgrpc import ( - opentracing "github.com/opentracing/opentracing-go" "context" + + opentracing "github.com/opentracing/opentracing-go" "github.com/opentracing/opentracing-go/ext" "github.com/opentracing/opentracing-go/log" "google.golang.org/grpc" @@ -14,9 +15,9 @@ import ( // // For example: // -// s := grpc.NewServer( -// ..., // (existing ServerOptions) -// grpc.UnaryInterceptor(otgrpc.OpenTracingServerInterceptor(tracer))) +// s := grpc.NewServer( +// ..., // (existing ServerOptions) +// grpc.UnaryInterceptor(otgrpc.OpenTracingServerInterceptor(tracer))) // // All gRPC server spans will look for an OpenTracing SpanContext in the gRPC // metadata; if found, the server span will act as the ChildOf that RPC @@ -33,12 +34,12 @@ func OpenTracingServerInterceptor(tracer opentracing.Tracer, optFuncs ...Option) info *grpc.UnaryServerInfo, handler grpc.UnaryHandler, ) (resp interface{}, err error) { - spanContext, err := extractSpanContext(ctx, tracer) - if err != nil && err != opentracing.ErrSpanContextNotFound { - // TODO: establish some sort of error reporting mechanism here. We - // don't know where to put such an error and must rely on Tracer - // implementations to do something appropriate for the time being. - } + spanContext, _ := extractSpanContext(ctx, tracer) + // if err != nil && !errors.Is(err, opentracing.ErrSpanContextNotFound) { + // // TODO: establish some sort of error reporting mechanism here. We + // // don't know where to put such an error and must rely on Tracer + // // implementations to do something appropriate for the time being. + // } if otgrpcOpts.inclusionFunc != nil && !otgrpcOpts.inclusionFunc(spanContext, info.FullMethod, req, nil) { return handler(ctx, req) @@ -76,9 +77,9 @@ func OpenTracingServerInterceptor(tracer opentracing.Tracer, optFuncs ...Option) // // For example: // -// s := grpc.NewServer( -// ..., // (existing ServerOptions) -// grpc.StreamInterceptor(otgrpc.OpenTracingStreamServerInterceptor(tracer))) +// s := grpc.NewServer( +// ..., // (existing ServerOptions) +// grpc.StreamInterceptor(otgrpc.OpenTracingStreamServerInterceptor(tracer))) // // All gRPC server spans will look for an OpenTracing SpanContext in the gRPC // metadata; if found, the server span will act as the ChildOf that RPC @@ -90,12 +91,12 @@ func OpenTracingStreamServerInterceptor(tracer opentracing.Tracer, optFuncs ...O otgrpcOpts := newOptions() otgrpcOpts.apply(optFuncs...) return func(srv interface{}, ss grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) error { - spanContext, err := extractSpanContext(ss.Context(), tracer) - if err != nil && err != opentracing.ErrSpanContextNotFound { - // TODO: establish some sort of error reporting mechanism here. We - // don't know where to put such an error and must rely on Tracer - // implementations to do something appropriate for the time being. - } + spanContext, _ := extractSpanContext(ss.Context(), tracer) + // if err != nil && !errors.Is(err, opentracing.ErrSpanContextNotFound) { + // // TODO: establish some sort of error reporting mechanism here. We + // // don't know where to put such an error and must rely on Tracer + // // implementations to do something appropriate for the time being. + // } if otgrpcOpts.inclusionFunc != nil && !otgrpcOpts.inclusionFunc(spanContext, info.FullMethod, nil, nil) { return handler(srv, ss) @@ -111,7 +112,7 @@ func OpenTracingStreamServerInterceptor(tracer opentracing.Tracer, optFuncs ...O ServerStream: ss, ctx: opentracing.ContextWithSpan(ss.Context(), serverSpan), } - err = handler(srv, ss) + err := handler(srv, ss) if err != nil { SetSpanTags(serverSpan, err, false) serverSpan.LogFields(log.String("event", "error"), log.String("message", err.Error())) diff --git a/shared.go b/shared.go index 9abd5ea..88f9cd1 100644 --- a/shared.go +++ b/shared.go @@ -8,10 +8,8 @@ import ( "google.golang.org/grpc/metadata" ) -var ( - // Morally a const: - gRPCComponentTag = opentracing.Tag{string(ext.Component), "gRPC"} -) +// Morally a const:. +var gRPCComponentTag = opentracing.Tag{Key: string(ext.Component), Value: "gRPC"} // metadataReaderWriter satisfies both the opentracing.TextMapReader and // opentracing.TextMapWriter interfaces. diff --git a/test/interceptor_test.go b/test/interceptor_test.go index 4626b68..a0970fd 100644 --- a/test/interceptor_test.go +++ b/test/interceptor_test.go @@ -1,6 +1,8 @@ package interceptor_test import ( + "context" + "errors" "io" "net" "testing" @@ -8,11 +10,11 @@ import ( "github.com/stretchr/testify/assert" - "context" - testpb "github.com/grpc-ecosystem/grpc-opentracing/go/otgrpc/test/otgrpc_testing" - "github.com/opentracing-contrib/go-grpc" + otgrpc "github.com/opentracing-contrib/go-grpc" + testpb "github.com/opentracing-contrib/go-grpc/test/otgrpc_testing" "github.com/opentracing/opentracing-go/mocktracer" "google.golang.org/grpc" + "google.golang.org/grpc/credentials/insecure" ) const ( @@ -21,13 +23,13 @@ const ( type testServer struct{} -func (s *testServer) UnaryCall(ctx context.Context, in *testpb.SimpleRequest) (*testpb.SimpleResponse, error) { - return &testpb.SimpleResponse{in.Payload}, nil +func (s *testServer) UnaryCall(_ context.Context, in *testpb.SimpleRequest) (*testpb.SimpleResponse, error) { + return &testpb.SimpleResponse{Payload: in.Payload}, nil } func (s *testServer) StreamingOutputCall(in *testpb.SimpleRequest, stream testpb.TestService_StreamingOutputCallServer) error { for i := 0; i < streamLength; i++ { - if err := stream.Send(&testpb.SimpleResponse{in.Payload}); err != nil { + if err := stream.Send(&testpb.SimpleResponse{Payload: in.Payload}); err != nil { return err } } @@ -38,7 +40,7 @@ func (s *testServer) StreamingInputCall(stream testpb.TestService_StreamingInput sum := int32(0) for { in, err := stream.Recv() - if err == io.EOF { + if errors.Is(err, io.EOF) { break } if err != nil { @@ -46,19 +48,19 @@ func (s *testServer) StreamingInputCall(stream testpb.TestService_StreamingInput } sum += in.Payload } - return stream.SendAndClose(&testpb.SimpleResponse{sum}) + return stream.SendAndClose(&testpb.SimpleResponse{Payload: sum}) } func (s *testServer) StreamingBidirectionalCall(stream testpb.TestService_StreamingBidirectionalCallServer) error { for { in, err := stream.Recv() - if err == io.EOF { + if errors.Is(err, io.EOF) { return nil } if err != nil { return err } - if err = stream.Send(&testpb.SimpleResponse{in.Payload}); err != nil { + if err = stream.Send(&testpb.SimpleResponse{Payload: in.Payload}); err != nil { return err } } @@ -80,6 +82,7 @@ type test struct { } func newTest(t *testing.T, e env) *test { + t.Helper() te := &test{ t: t, e: e, @@ -99,10 +102,22 @@ func newTest(t *testing.T, e env) *test { } te.srv = grpc.NewServer(sOpts...) testpb.RegisterTestServiceServer(te.srv, &testServer{}) - go te.srv.Serve(lis) + errChan := make(chan error, 1) + go func() { + errChan <- te.srv.Serve(lis) + }() + + // Check for immediate server startup errors + select { + case errServe := <-errChan: + if errServe != nil { + te.t.Fatalf("Failed to serve: %v", errServe) + } + default: + } // Set up a connection to the server. - cOpts := []grpc.DialOption{grpc.WithInsecure()} + cOpts := []grpc.DialOption{grpc.WithTransportCredentials(insecure.NewCredentials())} if e.unaryClientInt != nil { cOpts = append(cOpts, grpc.WithUnaryInterceptor(e.unaryClientInt)) } @@ -114,7 +129,7 @@ func newTest(t *testing.T, e env) *test { te.t.Fatalf("Failed to parse listener address: %v", err) } srvAddr := "localhost:" + port - te.cc, err = grpc.Dial(srvAddr, cOpts...) + te.cc, err = grpc.NewClient(srvAddr, cOpts...) if err != nil { te.t.Fatalf("Dial(%q) = %v", srvAddr, err) } @@ -127,6 +142,7 @@ func (te *test) tearDown() { } func assertChildParentSpans(t *testing.T, tracer *mocktracer.MockTracer) { + t.Helper() spans := tracer.FinishedSpans() assert.Equal(t, 2, len(spans)) if len(spans) != 2 { @@ -134,10 +150,15 @@ func assertChildParentSpans(t *testing.T, tracer *mocktracer.MockTracer) { } parent := spans[1] child := spans[0] - assert.Equal(t, child.ParentID, parent.Context().(mocktracer.MockSpanContext).SpanID) + parentContext, ok := parent.Context().(mocktracer.MockSpanContext) + if !ok { + t.Fatalf("Failed to assert parent context as mocktracer.MockSpanContext") + } + assert.Equal(t, child.ParentID, parentContext.SpanID) } func TestUnaryOpenTracing(t *testing.T) { + t.Parallel() tracer := mocktracer.New() e := env{ unaryClientInt: otgrpc.OpenTracingClientInterceptor(tracer), @@ -147,7 +168,7 @@ func TestUnaryOpenTracing(t *testing.T) { defer te.tearDown() payload := int32(0) - resp, err := te.c.UnaryCall(context.Background(), &testpb.SimpleRequest{payload}) + resp, err := te.c.UnaryCall(context.Background(), &testpb.SimpleRequest{Payload: payload}) if err != nil { t.Fatalf("Failed UnaryCall: %v", err) } @@ -156,6 +177,7 @@ func TestUnaryOpenTracing(t *testing.T) { } func TestStreamingOutputCallOpenTracing(t *testing.T) { + t.Parallel() tracer := mocktracer.New() e := env{ streamClientInt: otgrpc.OpenTracingStreamClientInterceptor(tracer), @@ -165,13 +187,13 @@ func TestStreamingOutputCallOpenTracing(t *testing.T) { defer te.tearDown() payload := int32(0) - stream, err := te.c.StreamingOutputCall(context.Background(), &testpb.SimpleRequest{payload}) + stream, err := te.c.StreamingOutputCall(context.Background(), &testpb.SimpleRequest{Payload: payload}) if err != nil { t.Fatalf("Failed StreamingOutputCall: %v", err) } for { resp, err := stream.Recv() - if err == io.EOF { + if errors.Is(err, io.EOF) { break } if err != nil { @@ -183,6 +205,7 @@ func TestStreamingOutputCallOpenTracing(t *testing.T) { } func TestStreamingInputCallOpenTracing(t *testing.T) { + t.Parallel() tracer := mocktracer.New() e := env{ streamClientInt: otgrpc.OpenTracingStreamClientInterceptor(tracer), @@ -193,8 +216,11 @@ func TestStreamingInputCallOpenTracing(t *testing.T) { payload := int32(1) stream, err := te.c.StreamingInputCall(context.Background()) + if err != nil { + t.Fatalf("Failed StreamingInputCall: %v", err) + } for i := 0; i < streamLength; i++ { - if err = stream.Send(&testpb.SimpleRequest{payload}); err != nil { + if err = stream.Send(&testpb.SimpleRequest{Payload: payload}); err != nil { t.Fatalf("Failed StreamingInputCall: %v", err) } } @@ -207,6 +233,7 @@ func TestStreamingInputCallOpenTracing(t *testing.T) { } func TestStreamingBidirectionalCallOpenTracing(t *testing.T) { + t.Parallel() tracer := mocktracer.New() e := env{ streamClientInt: otgrpc.OpenTracingStreamClientInterceptor(tracer), @@ -220,17 +247,27 @@ func TestStreamingBidirectionalCallOpenTracing(t *testing.T) { if err != nil { t.Fatalf("Failed StreamingInputCall: %v", err) } + errChan := make(chan error, 1) go func() { for i := 0; i < streamLength; i++ { - if err := stream.Send(&testpb.SimpleRequest{payload}); err != nil { - t.Fatalf("Failed StreamingInputCall: %v", err) + if err := stream.Send(&testpb.SimpleRequest{Payload: payload}); err != nil { + errChan <- err + return } } - stream.CloseSend() + if err := stream.CloseSend(); err != nil { + errChan <- err + return + } + errChan <- nil }() + + if err := <-errChan; err != nil { + t.Fatalf("Failed StreamingInputCall: %v", err) + } for { resp, err := stream.Recv() - if err == io.EOF { + if errors.Is(err, io.EOF) { break } if err != nil { @@ -242,6 +279,7 @@ func TestStreamingBidirectionalCallOpenTracing(t *testing.T) { } func TestStreamingContextCancellationOpenTracing(t *testing.T) { + t.Parallel() tracer := mocktracer.New() e := env{ streamClientInt: otgrpc.OpenTracingStreamClientInterceptor(tracer), @@ -252,7 +290,7 @@ func TestStreamingContextCancellationOpenTracing(t *testing.T) { payload := int32(0) ctx, cancel := context.WithCancel(context.Background()) - _, err := te.c.StreamingOutputCall(ctx, &testpb.SimpleRequest{payload}) + _, err := te.c.StreamingOutputCall(ctx, &testpb.SimpleRequest{Payload: payload}) if err != nil { t.Fatalf("Failed StreamingOutputCall: %v", err) } @@ -265,6 +303,14 @@ func TestStreamingContextCancellationOpenTracing(t *testing.T) { } parent := spans[0] child := spans[1] - assert.Equal(t, child.ParentID, parent.Context().(mocktracer.MockSpanContext).SpanID) - assert.True(t, parent.Tag("error").(bool)) + parentContext, ok := parent.Context().(mocktracer.MockSpanContext) + if !ok { + t.Fatalf("Failed to assert parent context as mocktracer.MockSpanContext") + } + assert.Equal(t, child.ParentID, parentContext.SpanID) + errorTag, ok := parent.Tag("error").(bool) + if !ok { + t.Fatalf("Failed to assert error tag as bool") + } + assert.True(t, errorTag) }