-
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
| /// | ||
| /// Note that this is overridden if OTEL_SDK_DISABLED is set and not empty. | ||
| pub(crate) fn otel_tracing_enabled() -> bool { | ||
| pub fn otel_tracing_enabled() -> bool { |
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.
It isn't immediately obvious why these visibility changes are necessary?
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 When you have a sec, would you mind addressing this?
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]>
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: