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

adaptation, stub: allow extra ttrpc client and server options. #67

Merged
merged 4 commits into from
Feb 1, 2024

Conversation

klihub
Copy link
Member

@klihub klihub commented Jan 30, 2024

This patch set adds support for passing extra ttrpc client and server options for both the runtime adaptation and the plugin stub. The primary purpose of this PR is to implement minimal plumbing which will allow the propagation of OpenTelemetry span information from runtimes to plugins, over NRI ttrpc connections,using the instrumentation interceptors implemented by https://github.com/containerd/otelttrpc.

This PR is an alternative to #68. This PR simply opens up the ttrpc client and server used by NRI for extra ttrpc options. This allows one to pass in the necessary options to enable otel trace span propagation (essentially using [github.com/containerd/]otelttrpc).

#68 on the other hand introduces two boolean options which tell NRI whether it internally should set up the necessary ttrpc options to enable otel trace span propagation over ttrpc. That brings in an indirect dependency on [github.com/containerd/]otelttrpc and OpenTelemetry itself.

I wanted to ask feedback about which of these alternative approaches others find more preferable, hence I filed both as draft PRs.

I think the biggest difference between these two approaches are in the blast radius of the resulting dependency changes, since #68 pulls in OpenTelemetry as an indirect dependency via otelttrpc.

Add WithTTRPCOptions() which can be used to pass extra client
and server options to ttrpc.

Signed-off-by: Krisztian Litkey <[email protected]>
Add WithTTRPCOptions() which can be used to pass extra client
and server options to ttrpc.

Signed-off-by: Krisztian Litkey <[email protected]>
Signed-off-by: Krisztian Litkey <[email protected]>
Signed-off-by: Krisztian Litkey <[email protected]>
@codecov-commenter
Copy link

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (6f5a4d2) 64.50% compared to head (1b84c72) 64.36%.

❗ Current head 1b84c72 differs from pull request most recent head 45b9e3f. Consider uploading reports for the commit 45b9e3f to get more accurate results

Files Patch % Lines
pkg/adaptation/adaptation.go 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #67      +/-   ##
==========================================
- Coverage   64.50%   64.36%   -0.14%     
==========================================
  Files           9        9              
  Lines        1834     1841       +7     
==========================================
+ Hits         1183     1185       +2     
- Misses        500      505       +5     
  Partials      151      151              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cpuguy83
Copy link
Member

I think I like this one better?

@fuweid
Copy link
Member

fuweid commented Jan 31, 2024

+1 for this one

@klihub klihub marked this pull request as ready for review January 31, 2024 06:39
@klihub
Copy link
Member Author

klihub commented Jan 31, 2024

@cpuguy83 @fuweid ACK, closed #68 and removed the draft status from this one.

@klihub klihub changed the title Allow extra ttrpc client and server options for both runtime/adaptation and plugin/stub. adaptation, stub: allow extra ttrpc client and server options. Jan 31, 2024
Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@fuweid fuweid requested a review from dmcgowan February 1, 2024 06:24
@fuweid fuweid merged commit e6fb9fe into containerd:main Feb 1, 2024
8 checks passed
@klihub klihub deleted the devel/extra-ttrpc-options branch February 2, 2024 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants