Move to OpenTelemetry (#735)

[Alvise88]

Signed-off-by: Alvise vitalvise@gmail.com

[Alvise88]

There is no tagged buildkit release with opentelemetry integration

To fix (horrible) i have to do:

github.com/moby/buildkit => github.com/moby/buildkit v0.8.2-0.20210702052944-9cf28dcf2640

and modify the check to use only the first result:

check-buildkit-version:
	@test \
		"$(shell grep buildkit ./go.mod | head -1 | cut -d' ' -f2)" = \
		"$(shell grep ' = "v' ./util/buildkitd/buildkitd.go | sed -E 's/^.*version.*=.*\"(v.*)\"/\1/' )" \
		|| { echo buildkit version mismatch go.mod != util/buildkitd/buildkitd.go ; exit 1; }

The difference remains at runtime

[aluzzardi]

Thanks @Alvise88, will review shortly!

Regarding buildkit: maybe we can continue using the same version of buildkit, and upgrade once the new one comes out. We will lose some tracing information but not too much.

[Alvise88]

Thanks @Alvise88, will review shortly!

Regarding buildkit: maybe we can continue using the same version of buildkit, and upgrade once the new one comes out. We will lose some tracing information but not too much.

Ok, i disabled tracing to buildkit and reverted to v0.8.3.

[aluzzardi]

@Alvise88 I'm looking at moby/buildkit#2152 trying to understand how they integrated OpenTelemetry to BuildKit.

I see for instance they're using trace.SpanFromContext(ctx) (from go.opentelemetry.io/otel/trace), whereas in this PR it's otel.Tracer() and I'm confused about the difference.

Do you have an idea of what's the difference?

Also, did you get a chance to try with Jaeger, and if so, are the spans from a dagger command combined into a single trace?

Thanks!

[Alvise88]

@Alvise88 I'm looking at moby/buildkit#2152 trying to understand how they integrated OpenTelemetry to BuildKit.

I see for instance they're using trace.SpanFromContext(ctx) (from go.opentelemetry.io/otel/trace), whereas in this PR it's otel.Tracer() and I'm confused about the difference.

Do you have an idea of what's the difference?

Also, did you get a chance to try with Jaeger, and if so, are the spans from a dagger command combined into a single trace?

Thanks!

Hi, tomorrow i will show you how spans are genereted.
In opentelemetry you can define different traces; this should allow you to selectively disable tracing.

I think this instruction span := trace.SpanFromContext(ctx) permits you to retrive the active span in the context instead tr.Start permits you to create a child span.

I replaced your opentracing calls by analogy, i'm not 100% sure it's correct:
opentracing.StartSpanFromContext -> tr.Start
opentracing.SpanFromContext -> trace.SpanFromContext(ctx) (like buildkit)

I follow this documentation

[Alvise88]

Hi @aluzzardi, i used docker compose to test tracing, the following images show you the result:

OpenTracing

OpenTelemetry

OpenTelemetry "Details"

New Buildkit integration

[aluzzardi]

Thanks for the explanation, and the screenshots you sent look good!

For some reason, with OpenTracing, Dagger and Buildkitd show up as separate (different color in Jaeger), but they appear the same with the OpenTelemetry version, not sure why.

Here's the OpenTracing details screenshot:

However it's a very minor difference, I think we don't care

[Alvise88]

I think the new integration uses the same traceprovider in the buildkit client:

append(opts, bk.WithTracerProvider(span.TracerProvider()))

currently i used only one traceprovider named Dagger, i don't know if it is better to instantiate a new provider named buildkit or consider traces as part of Dagger; the colors are related with the name of the provider

[aluzzardi]

I think the PR is good as is!

Good for me to merge as soon as you remove "WIP" from the title :)

Buildkit 0.9.0 is about to come out (-rc1 is out already), which means we'll be able to get OpenTracing support from there as well.

[Alvise88]

I think the PR is good as is!

Good for me to merge as soon as you remove "WIP" from the title :)

Buildkit 0.9.0 is about to come out (-rc1 is out already), which means we'll be able to get OpenTracing support from there as well.

Ok, i removed the wip annotation; tell me if you want to change the buildkit version or if it is needed a rebase.

[aluzzardi]

Okay I just reviewed the code, left a few comments, after that it's good to go.

Ok, i removed the wip annotation; tell me if you want to change the buildkit version or if it is needed a rebase.

I think it's ok, we will change the buildkit version once it comes out

[netlify]

✔️ Deploy Preview for devel-docs-dagger-io ready!

🔨 Explore the source changes: e34c4ba

🔍 Inspect the deploy log: https://app.netlify.com/sites/devel-docs-dagger-io/deploys/60e7fe9c1acebe0007745dc8

😎 Browse the preview: https://deploy-preview-778--devel-docs-dagger-io.netlify.app