OpenTracing First Impl

[sjug]

Intentionally not squashed as I'm not sure if the PR should be broken up further.
There is also a relatively large corresponding PR to containers/image.

Nothing crazy going on, just a simple implementation of OpenTracing.

[mheon]

Nice! I'll do a more thorough review later, but a once-over finds nothing wrong

[mheon]

/approve

[mheon]

/ok-to-test

[baude]

the test failures look legit on PAPR. Is there an issue with vendoring of code?

[sjug]

@baude Ah you're right I didn't vendor anything in, thanks.

[sjug]

/retest

[baude]

@sjug looks like a gofmt error..

[TomSweeneyRedHat]

gofmt errors. Wonder if we're getting bit by the version diff of gofmt between go1.10 and go1.11.

[sjug]

Thanks, I missed that. Not sure what was the cause as I'm on 1.11+ everywhere.

[sjug]

What do I need for does not have a valid DCO?

[rhatdan]

All of you PR's need to be signed
git commit -a --amend -s

[rh-atomic-bot]

☔ The latest upstream changes (presumably #1639) made this pull request unmergeable. Please resolve the merge conflicts.

[giuseppe]

could we split the update for vendor/github.com/urfave/cli/context.go to a different patch? It will make the review easier, and it will also be easier to bisect in future

[sjug]

I agree @giuseppe, should I separate vendor/github.com/urfave/cli/context.go into a different commit or a different PR altogether?

[sjug]

Thanks @rhatdan, do you think we could merge this sometime this week?

[mheon]

I'll drop a review on Monday, so if we can grab one more it should be good to merge.

…

On Sun, Feb 17, 2019, 07:53 Sebastian Jug ***@***.*** wrote: Thanks @rhatdan <https://github.com/rhatdan>, do you think we could merge this sometime this week? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1692 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHYHCIbF-X0gA-apbK-TQLlI456M9850ks5vOVDXgaJpZM4XzWiy> .

[rh-atomic-bot]

☔ The latest upstream changes (presumably #2295) made this pull request unmergeable. Please resolve the merge conflicts.

[giuseppe]

LGTM

[TomSweeneyRedHat]

IDK what's up, but it looks like vendor isn't happy with this atm.

[sjug]

@TomSweeneyRedHat I can't reproduce whatever the gating failure is? As I mentioned up here.
Any suggestions for how to fix this?

[mheon]

@vrothberg Mind taking a look at the vendoring failure in gating?

[mheon]

Code LGTM

[vrothberg]

@vrothberg Mind taking a look at the vendoring failure in gating?

The scripts claims inconsistencies with the following files:

D vendor/github.com/opentracing/opentracing-go/LICENSE D vendor/github.com/opentracing/opentracing-go/README.md D vendor/github.com/opentracing/opentracing-go/ext/tags.go D vendor/github.com/opentracing/opentracing-go/globaltracer.go D vendor/github.com/opentracing/opentracing-go/gocontext.go D vendor/github.com/opentracing/opentracing-go/log/field.go D vendor/github.com/opentracing/opentracing-go/log/util.go D vendor/github.com/opentracing/opentracing-go/noop.go D vendor/github.com/opentracing/opentracing-go/propagation.go D vendor/github.com/opentracing/opentracing-go/span.go D vendor/github.com/opentracing/opentracing-go/tracer.go M vendor/github.com/uber/jaeger-lib/metrics/factory.go M vendor/github.com/uber/jaeger-lib/metrics/metrics.go ?? vendor/github.com/uber/jaeger-lib/metrics/histogram.go

One issue is that github.com/uber/jaeger-lib is vendoring a branch, which is something we don't do/allow anymore for various reasons such as reproducibility, maintainability and also for packaging. Some distributions do not use the ./vendor folder but package all dependencies separately. Having a branch makes all these things very hard.

Another issue seems to be that too many files are added to vendor/github.com/opentracing/opentracing-go/. One way to resolve the issue is running make vendor locally and adding the remaining changes. After that, the script should pass.

[sjug]

@vrothberg Thank you, I have updated the opentracing deps to use commits instead and run make vendor as suggested.

[TomSweeneyRedHat]

Thanks for the assist @vrothberg !

[vrothberg]

A pleasure :) The script is fairly new. Such feedback is really helpful in improving documentation and the output of the script as well.

[rhatdan]

/lgtm

[sjug]

Thanks @rhatdan and all reviewers! This PR initializes the tracing spans in the top level functions (run, pull, etc), but then we also need the further containers/image PR to continue the trace into the libs as well.