nri: enable otel tracing

[klihub]

What type of PR is this?

/kind other

What this PR does / why we need it:

This PR updates our NRI dependency enabling OpenTelemetry trace context propagation over ttrpc connections. Additionally the PR also enables automatic OpenTelemetry instrumentation of NRI ttrpc calls when tracing is globally enabled in cri-o.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

- Enable automatic OpenTelemetry instrumentation of ttrpc calls to NRI plugins when tracing is globally enabled in cri-o.
- Bumping our NRI deps also fixes a bug where starting a cri-o launched NRI plugin could cause the accidental closing of an unrelated fd (socket, file, etc.) due to an fd double-close bug.

[klihub]

/retest-required

[codecov]

Codecov Report

Merging #7728 (d955623) into main (e9febd3) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7728   +/-   ##
=======================================
  Coverage   47.95%   47.95%           
=======================================
  Files         146      146           
  Lines       16274    16274           
=======================================
  Hits         7804     7804           
  Misses       7517     7517           
  Partials      953      953           

[saschagrunert]

Thank you! Code LGTM

[saschagrunert]

/retest

[klihub]

Thank you! Code LGTM

@saschagrunert ttrpc got tagged a new version. Updated deps to point the latest tag. That cost me your earlier lgtm.

[kwilczynski]

Thank you! Code LGTM

@saschagrunert ttrpc got tagged a new version. Updated deps to point the latest tag. That cost me your earlier lgtm.

@klihub, might be a silly question, but... can we use the normal OTEL package we already have? Or does it have to be ttrpc explicitly (e.g., no way to adapt the other one in any way, etc.)?

[klihub]

Thank you! Code LGTM

@saschagrunert ttrpc got tagged a new version. Updated deps to point the latest tag. That cost me your earlier lgtm.

@klihub, might be a silly question, but... can we use the normal OTEL package we already have? Or does it have to be ttrpc explicitly (e.g., no way to adapt the other one in any way, etc.)?

otelttrpc tries to be for ttrpc what otelgrpc is for grpc. Currently it implements instrumenting unary client and server interceptors which enable 2 things:

1. the automatic injection of otel traces for each unary RPC call invoked and served over ttrpc (we still lack support for streaming calls), and
2. the automatic propagation of otel trace context (span ID/info) over the IPC transport used for ttrpc, so that related/resulting computation on the other side of the IPC link gets a proper parent span ID and gets properly nested in visualization under the right originating/triggering event

So otelttrpc is not a replacement for otel, it is a helper package which implements instrumenting interceptors so that we get ttrpc closer to grpc wrt. otel tracing transparency.

With this PR in place, if tracing is enabled any related processing in NRI plugins should correctly show up in the traces. If a plugin is uninstrumented, a single (unary client call) should show up. If the plugin itself is instrumented, more detailed spans should be available as child spans of this.

[kwilczynski]

[...]

@klihub, might be a silly question, but... can we use the normal OTEL package we already have? Or does it have to be ttrpc explicitly (e.g., no way to adapt the other one in any way, etc.)?

otelttrpc tries to be for ttrpc what otelgrpc is for grpc. Currently it implements instrumenting unary client and server interceptors which enable 2 things:

Makes sense. Since NRI uses ttrpc as its protocol of choice. Thank you!

[kwilczynski]

/approve
/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: klihub, kwilczynski, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [klihub,kwilczynski,saschagrunert]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[kwilczynski]

@klihub, sorry to appear out of the blue on this closed Pull Request, but I have a question.

The containerd/otelttrpc package, which we now use, seems to be causing some build issues when we build CRI-O binary packages (RPMs) for use in OpenShift per:

_output/src/github.com/cri-o/cri-o/vendor/github.com/containerd/otelttrpc/interceptor.go:50:2: code in directory /builddir/build/BUILD/cri-o-4fa897e3b639c3bf19ad0152f2429162548e5698/_output/src/github.com/cri-o/cri-o/vendor/github.com/containerd/otelttrpc/internal expects import "github.com/containerd/ttrpc/otelttrpc"

This seems to be due to the "import path checking" feature you are using there.

Just wondering, do you think this is something that you need to leverage in this specific package?

[klihub]

@klihub, sorry to appear out of the blue on this closed Pull Request, but I have a question.

The containerd/otelttrpc package, which we now use, seems to be causing some build issues when we build CRI-O binary packages (RPMs) for use in OpenShift per:

_output/src/github.com/cri-o/cri-o/vendor/github.com/containerd/otelttrpc/interceptor.go:50:2: code in directory /builddir/build/BUILD/cri-o-4fa897e3b639c3bf19ad0152f2429162548e5698/_output/src/github.com/cri-o/cri-o/vendor/github.com/containerd/otelttrpc/internal expects import "github.com/containerd/ttrpc/otelttrpc"

This seems to be due to the "import path checking" feature you are using there.

Just wondering, do you think this is something that you need to leverage in this specific package?

@kwilczynski No, I can't see why we would need any of that. And actually it is more of an accident (IOW, me not being aware of the consequences) that we have those in place. I'll file a PR to get rid of them.

[klihub]

@kwilczynski Filed here: containerd/otelttrpc#1. Once that gets merged, I'll file corresponding PRs to update the cri-o otelttrpc dependency

[kwilczynski]

@kwilczynski Filed here: containerd/otelttrpc#1. Once that gets merged, I'll file corresponding PRs to update the cri-o otelttrpc dependency

@klihub, thank you on behalf of our build team! I appreciate a super fast response from you! 😄