-
Notifications
You must be signed in to change notification settings - Fork 289
WASI OTel #3346
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
base: main
Are you sure you want to change the base?
WASI OTel #3346
Conversation
Signed-off-by: Caleb Schoepp <[email protected]> fix: update opentelemetry version Signed-off-by: Andrew Steurer <[email protected]> Fix out of date Cargo.lock from bad rebase Signed-off-by: Caleb Schoepp <[email protected]> feat(factor-otel): adding metrics Signed-off-by: Andrew Steurer <[email protected]> feat(factor-otel): refactoring otel conversions Signed-off-by: Andrew Steurer <[email protected]> fix(telemetry): update BatchLogProcessor to be async Signed-off-by: Andrew Steurer <[email protected]> feat(factor-otel): refactoring WIT and updating conversions Signed-off-by: Andrew Steurer <[email protected]> fix(factor-otel): updating WIT Signed-off-by: Andrew Steurer <[email protected]> feat(factor-otel): updating WIT and conversions Signed-off-by: Andrew Steurer <[email protected]> feat(factor-otel): small refactors Signed-off-by: Andrew Steurer <[email protected]> fix(factor-otel): rebasing wasi-otel branch on main Signed-off-by: Andrew Steurer <[email protected]>
|
@lann I think you would likely be the most relevant reviewer for this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was mostly just a shallow pass here with a few surface-level comments.
Looking at how this code is working, I think there may be a viable approach that wouldn't require changes to the outbound factors. I only have a minute right now but briefly:
- Use a task-local to manage state through the host-guest-host burger
- Use an otel span processor that uses that task-local to conditionally reparent spans
crates/world/src/conversions.rs
Outdated
| } | ||
| } | ||
|
|
||
| mod otel { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My immediate hot take is that we should break this out into a separate wasi-otel crate. We've been discussing removing this spin-world crate entirely and I think these conversion impls are a good example of why we'd rather split up these import bindings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a separate crate for wasi-otel; however, the src/lib.rs file is an exact copy of the world/src/lib.rs file. It works; however, I could use some assistance tailoring the bindgen! macro to wasi-otel.
Signed-off-by: Andrew Steurer <[email protected]>
crates/factor-otel/src/lib.rs
Outdated
| impl OtelFactor { | ||
| pub fn new() -> anyhow::Result<Self> { | ||
| // This is a hack b/c we know the version of this crate will be the same as the version of Spin | ||
| let spin_version = env!("CARGO_PKG_VERSION").to_string(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably just take the version as an argument to the new constructor and do this 'hack' from inside the actual Spin crate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I did this correctly. Will you double check and let me know?
crates/factor-otel/src/lib.rs
Outdated
| /// This MUST only be called from a factor host implementation function that is instrumented. | ||
| /// | ||
| /// This MUST be called at the very start of the function before any awaits. | ||
| pub fn reparent_tracing_span(&self) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean the host loses these spans for its own tracing? Ideally we would emit these spans both for host and guest tracing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@calebschoepp Will you address this when you have a sec?
| use opentelemetry_sdk::metrics::exporter::PushMetricExporter; | ||
| use opentelemetry_sdk::trace::SpanProcessor; | ||
| use tracing_opentelemetry::OpenTelemetrySpanExt; | ||
| // TODO: This feels weird. I'm wondering if it can be fixed by wrangling the bindgen macro in `crates/wasi_otel/lib.rs`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: comments like this saying things like "this feels weird" is rarely helpful. It's usually not clear to readers (likely even the author themselves in the future) what feels weird. We should either address this issue now, write a more descriptive comment, or remove the comment entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this was intended as something to solicit help from reviewers, not to be merged. If it's not something we want to worry about, happy to remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This app doesn't seem to do anything specific with wasi. Why do we need this instead of using any of the existing components?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@calebschoepp Will you address this when you have a sec?
| http = "0.2" | ||
| opentelemetry = "0.30.0" | ||
| opentelemetry_sdk = "0.30.0" | ||
| opentelemetry-wasi = { git = "https://github.com/calebschoepp/opentelemetry-wasi", rev = "60df119fbc03ed83d0e2cc1300f6a8eb055d23f7" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the plan is to upstream this back somewhere? Do we know where?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@calebschoepp Will you address this when you have a sec?
| // TODO: Test what happens if start is called but not end | ||
| // TODO: Test what happens if end is called but not start | ||
| // TODO: What happens if child span outlives parent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's answer all these questions
Co-authored-by: Ryan Levick <[email protected]>
Co-authored-by: Ryan Levick <[email protected]>
Signed-off-by: Andrew Steurer <[email protected]>
Signed-off-by: Andrew Steurer <[email protected]>
Overview
This is an implementation of WASI OTel which gives guest applications the ability to export traces and metrics (and eventually logs) with OpenTelemetry SDKs.
We see two options for adding this to Spin:
To help ease adoption, we would prefer to pass a flag to spin up, but we know there was some previous work around this with WASI P3 so we're happy to defer to that process.
Some additional notes:
Cargo.toml, you will notice that certain experimental features are being enabled for theopentelemetry-sdkdependency, and that we are usingreqwest-clientinstead ofreqwest-blocking-clientin theopentelemetrydependency. These are all to prevent runtime errors that come from the default OpenTelemetry dependency configurations conflicting with Spin's async runtime.wasi-otelis a phase 0 proposal but we are presenting it to the WASI subgroup this month in the hopes of moving it to phase 1.Usage
To try the features added in this PR, do the following: