Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

builder-next: enable OTLP tracing for history records #45579

Merged
merged 1 commit into from Jun 20, 2023

Conversation

tonistiigi
Copy link
Member

This enables picking up OTLP tracing context for the gRPC requests.

Also sets up the in-memory recorder that BuildKit History API can use to store the traces associated with specific build in a database after build completes.

This doesn't enable Jaeger tracing endpoints from env but this can be easily enabled by adding another import if maintainers want it.

@cpuguy83
Copy link
Member

I actually just started a branch this passed week to enable tracing on the HTTP API + OTLP exporters.

@thaJeztah
Copy link
Member

discussed with @tonistiigi and this should be ready for review

@thaJeztah
Copy link
Member

@tonistiigi looks like there's an issue with the vendor check;

diff --git a/vendor.mod b/vendor.mod
index ddbb0b6fd1..ebb8e1ac67 100644
--- a/vendor.mod
+++ b/vendor.mod
@@ -46,6 +46,7 @@ require (
 	github.com/google/go-cmp v0.5.9
 	github.com/google/uuid v1.3.0
 	github.com/gorilla/mux v1.8.0
+	github.com/grpc-ecosystem/go-grpc-middleware v1.3.0
 	github.com/hashicorp/go-immutable-radix v1.3.1
 	github.com/hashicorp/go-memdb v1.3.2
 	github.com/hashicorp/go-multierror v1.1.1
@@ -87,6 +88,9 @@ require (
 	github.com/vishvananda/netlink v1.2.1-beta.2
 	github.com/vishvananda/netns v0.0.2
 	go.etcd.io/bbolt v1.3.7
+	go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/o
telgrpc v0.29.0
+	go.opentelemetry.io/otel v1.4.1
+	go.opentelemetry.io/otel/trace v1.4.1
 	golang.org/x/mod v0.10.0
 	golang.org/x/net v0.8.0
 	golang.org/x/sync v0.1.0
Error: The operation was canceled.

Comment on lines +73 to +78
if strings.HasSuffix(info.FullMethod, "opentelemetry.proto.collector.trace.v1.TraceService/Export") {
return handler(ctx, req)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this particular gRPC request method need to be special-cased to be excluded from tracing? I'd like to see a code comment with the explanation.

Any reason why the special-casing is done with bespoke code rather than otelgrpc.WithInterceptorFilter?

otelgrpc.WithInterceptorFilter(filters.Not(filters.FullMethodName("/opentelemetry.proto.collector.trace.v1.TraceService/Export")))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just discussed in the maintainers call, and it's to prevent an infinite loop (but Tonis will add a comment describing the details)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(but maybe newer versions of otel can clean this up; he's looking into it)

(don't shoot the messenger)

This enables picking up OTLP tracing context for the gRPC
requests.

Also sets up the in-memory recorder that BuildKit History API
can use to store the traces associated with specific build
in a database after build completes.

This doesn't enable Jaeger tracing endpoints from env
but this can be easily enabled by adding another import if
maintainers want it.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@thaJeztah
Copy link
Member

@thaJeztah thaJeztah added this to the 25.0.0 milestone Jun 20, 2023
@thaJeztah thaJeztah merged commit 404160a into moby:master Jun 20, 2023
101 of 102 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants