containerd / ttrpc

GRPC for low-memory environments
Apache License 2.0
559 stars 80 forks source link

Add ttrpc interceptors #134

Closed swagatbora90 closed 1 year ago

swagatbora90 commented 1 year ago

This adds otel ttrpc interceptors for both client and server.

Continuation of https://github.com/containerd/ttrpc/pull/110 which has now been closed.

Signed-off-by: Swagat Bora sbora@amazon.com

swagatbora90 commented 1 year ago
# go.opentelemetry.io/otel/internal/attribute
Error: /home/runner/go/pkg/mod/go.opentelemetry.io/otel@v1.13.0/internal/attribute/attribute.go:26:6: missing function body
Error: /home/runner/go/pkg/mod/go.opentelemetry.io/otel@v1.13.0/internal/attribute/attribute.go:26:16: syntax error: unexpected [, expecting (
Error: /home/runner/go/pkg/mod/go.opentelemetry.io/otel@v1.13.0/internal/attribute/attribute.go:28:2: syntax error: non-declaration statement outside function body
Error: /home/runner/go/pkg/mod/go.opentelemetry.io/otel@v1.13.0/internal/attribute/attribute.go:34:6: missing function body
Error: /home/runner/go/pkg/mod/go.opentelemetry.io/otel@v1.13.0/internal/attribute/attribute.go:34:13: syntax error: unexpected [, expecting (
Error: /home/runner/go/pkg/mod/go.opentelemetry.io/otel@v1.13.0/internal/attribute/attribute.go:39:6: zero redeclared in this block
Error:  /home/runner/go/pkg/mod/go.opentelemetry.io/otel@v1.13.0/internal/attribute/attribute.go:27:6: previous declaration
Error: /home/runner/go/pkg/mod/go.opentelemetry.io/otel@v1.13.0/internal/attribute/attribute.go:40:2: syntax error: non-declaration statement outside function body
note: module requires Go 1.18
FAIL    github.com/containerd/ttrpc/tracing/otelttrpc [build failed]

Looks like GH actions are only running for go 1.17. There is an open PR to enable latest go version PR#124

estesp commented 1 year ago

Should be able to rebase now and get the updated CI run with Go 1.20/1.19

klihub commented 1 year ago

@swagatbora90 @estesp Can I help somehow to push this PR forward ? Picking this up and fixing the remaining issues ? Doing some other shoveling ?

I had very similar ideas than presented here, namely to start from the otelgrpc instrumentator, then replace any gRPC-specific code with the ttRPC equivalent. You can see my WiP RFC PR here.

I was trying to make this idea clearer/take this idea further there: I tried to make otelttrpc as verbatim close to otelgrpc as possible. The goal with that was/is to minimize the mental cost of context switching between the two, with the hope that we could tail-surf any bugfixes or other maintenance updates that the grpc folks make over there.

I'd like to get ttRPC opentelemetry instrumentation support up and running because we'd like to add instrumentation to the NRI processing pipeline, including external plugins which live behind a ttRPC connection.

I can try to finalize my otelgrpc-based WiP PR , or I can try to pick this one up and see if I can fix the remaining problems, if that is considered a better way forward. If I can help in any way, please let me know.

swagatbora90 commented 1 year ago

@klihub We have seen a number of request to add ttrpc interceptors for otel, so appreciate your willingness to work on it. I haven't been able to dedicate much time to it recently, so I am okay if you want to work on this and take this forward. The changes here are very similar to what you have in your PR, so feel free to close this PR and add a new one.

Also took a look at your PR, opentelemetry-go-contrib is not accepting new instrumentation PRs. there was a previous attempt which got closed. ttrpc package seems like good place to host the interceptors, perhaps in its own contrib section.

As for this PR, I was able to get the interceptors working and we can get traces for ttrpc calls. However,the otelexample in the PR still needs some work. otelgrpc has a similar example that I thought could be useful. Ofcourse we can add that later.

klihub commented 1 year ago

Closing, will opening an alternative one with less divergence from the gRPC one (which was also the starting point for this one).