search

lightstep / lightstep-tracer-go

The Lightstep distributed tracing library for Go
https://lightstep.com
MIT License
98 stars 54 forks source link

Migrate dependency management from dep to go modules #214

Closed bruceadowns closed 5 years ago

bruceadowns commented 5 years ago
bruceadowns commented 5 years ago

cross reference issue https://github.com/lightstep/lightstep-tracer-common/issues/37

bruceadowns commented 5 years ago

The build error looks like an unrelated race condition.

i.e.

$ go test -race -v
=== RUN   TestLightstepTracerGo
Running Suite: LightstepTracerGo Suite
======================================
Random Seed: 1558394532
Will run 67 of 67 specs

••••••••••••••••••••••••••••••••••••••••••==================
WARNING: DATA RACE
Write at 0x00c000164948 by goroutine 82:
  github.com/lightstep/lightstep-tracer-go.(*tracerImpl).preFlush()
      /Users/downsb/go/src/github.com/lightstep/lightstep-tracer-go/tracer.go:309 +0x2f4
  github.com/lightstep/lightstep-tracer-go.(*tracerImpl).Flush()
      /Users/downsb/go/src/github.com/lightstep/lightstep-tracer-go/tracer.go:244 +0xa2
  github.com/lightstep/lightstep-tracer-go.(*tracerImpl).reportLoop()
      /Users/downsb/go/src/github.com/lightstep/lightstep-tracer-go/tracer.go:404 +0x2b1
...
jmacd commented 5 years ago

Thanks!

I know @iredelmeier and @austinlparker are at conferences this week, so I can take over.

I haven't been able to reproduce the race condition yet. The one thing I want to ask for is that we preserve the legacy Gopkg.* configuration, for now.

bruceadowns commented 5 years ago

I haven't been able to reproduce the race condition yet.

Thanks for tracking @jmacd!

I am able to reproduce each time once upgrading to go modules. This certainly seems unrelated, but I will experiment with existing master to see what I can identify.

bruceadowns commented 5 years ago

I haven't been able to reproduce the race condition yet.

I am able to reproduce each time once upgrading to go modules. This certainly seems unrelated, but I will experiment with existing master to see what I can identify.

This is a little scary, but after experimenting locally I am no longer able to reproduce the race condition. And circleci also succeeds.

bruceadowns commented 5 years ago

The one thing I want to ask for is that we preserve the legacy Gopkg.* configuration, for now.

I restored Gopkg.lock/Gopkg.toml files, though they are now deprecated.

jmacd commented 5 years ago

I noticed a new dependency on golang/protobuf 1.3.0, which I think we don't need b/c we're using gogo exclusively in the library. This made me compare with the version here:

https://github.com/lightstep/lightstep-tracer-go/blob/iredelmeier/use-go-modules/go.mod

Do you feel that we really need all the new dependencies in your .mod file?

bruceadowns commented 5 years ago

I noticed a new dependency on golang/protobuf 1.3.0, which I think we don't need b/c we're using gogo exclusively in the library. This made me compare with the version here:

https://github.com/lightstep/lightstep-tracer-go/blob/iredelmeier/use-go-modules/go.mod

Do you feel that we really need all the new dependencies in your .mod file?

Good catch, let me see if I can reconcile.

(note that I am following the prescribed migration advice from https://github.com/golang/go/wiki/Modules#how-to-define-a-module)

In looking through the existing Gopkg.lock, I do see an (indirect?) dependency on github.com/golang/protobuf. See https://github.com/lightstep/lightstep-tracer-go/blob/master/Gopkg.lock#L16. My guess is it's pulled in as a transitive dependency via lightstep-tracer-common.

jmacd commented 5 years ago

@iredelmeier Will you take a look?

jmacd commented 5 years ago

Thanks @bruceadowns and @iredelmeier