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

feat(tracing): add tracing instruments to containerd-shim-wasm #564

Merged
merged 1 commit into from
May 16, 2024

Conversation

Mossaka
Copy link
Member

@Mossaka Mossaka commented Apr 17, 2024

this commit adds tracing instrument macros to functions in the containerd-shim-wasm crate to capture spans of each function including its parameters and results.

these spans can be in turn be collected using tracing-opentelemetry and opentelemetry SDK at the shim binary level and output to collectors like Jeager endpoint.

Demo: https://github.com/Mossaka/runwasi/tree/otel-wasmtime

@Mossaka Mossaka marked this pull request as draft April 17, 2024 15:05
@Mossaka Mossaka mentioned this pull request Apr 17, 2024
6 tasks
@Mossaka Mossaka marked this pull request as ready for review May 6, 2024 02:41
@Mossaka Mossaka changed the title feat(otel): add instruments to containerd-shim-wasm feat(tracing): add tracing instruments to containerd-shim-wasm May 6, 2024
this commit adds tracing instrument macros to functions in the
containerd-shim-wasm crate to capture spans of each function
including its parameters and results.

these spans can be in turn be collected using tracing-opentelemetry
and opentelemetry SDK at the shim binary level and output to collectors
like Jeager endpoint.

Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>
Copy link
Contributor

@jsturtevant jsturtevant left a comment

Choose a reason for hiding this comment

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

A few questions about the level and ability to opt/in out. Although we support it not everyone might have a OTEL environment set up, if they don't do they pay performance hit for it being enabled?

@@ -36,6 +37,7 @@ macro_rules! revision {
};
}

#[instrument(skip_all, parent = Span::current(), level= "Info")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want all of these at Info? Is there a performance overhead?

@@ -44,6 +44,7 @@ ttrpc = "0.8.0"
wat = "1.206"
windows-sys = "0.52"
serial_test = "2"
tracing = "0.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be an optional feature in the case downstream shims don't use it? containerd/rust-extensions#260

@jsturtevant
Copy link
Contributor

@Mossaka and I chatted offline, there isn't a major overhead (>1ms) so we will go ahead with this and can always adjust as need.

Copy link
Contributor

@jsturtevant jsturtevant left a comment

Choose a reason for hiding this comment

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

LGTM

@jsturtevant jsturtevant merged commit eab0db3 into containerd:main May 16, 2024
51 checks passed
@Mossaka Mossaka deleted the otel branch May 16, 2024 22:31
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.

2 participants