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(wasi-observe): WASI Observe factor implementation #2787

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

calebschoepp
Copy link
Collaborator

@calebschoepp calebschoepp commented Aug 29, 2024

  • Adds a new WASI Observe factor.
  • Adds a suite of integration tests for the factor.
  • Makes all other factors take an optional dependency on the ObserveFactor to handle proper span nesting of other factors.

@calebschoepp calebschoepp force-pushed the rework-wasi-observe branch 2 times, most recently from f9de9d9 to 9f26551 Compare August 29, 2024 15:25
@calebschoepp calebschoepp force-pushed the rework-wasi-observe branch 2 times, most recently from 0bbb37b to 4461b03 Compare September 5, 2024 17:38
@calebschoepp calebschoepp force-pushed the rework-wasi-observe branch 2 times, most recently from 43dcddc to 1741e59 Compare September 11, 2024 20:10
tests/integration.rs Outdated Show resolved Hide resolved
@calebschoepp calebschoepp force-pushed the rework-wasi-observe branch 4 times, most recently from eb8dc27 to f1487d5 Compare September 16, 2024 20:08
@calebschoepp calebschoepp mentioned this pull request Sep 17, 2024
@calebschoepp calebschoepp force-pushed the rework-wasi-observe branch 4 times, most recently from 5f2c7c1 to 810d05f Compare September 17, 2024 22:04
Cargo.toml Outdated Show resolved Hide resolved

impl From<tracer::SpanContext> for opentelemetry::trace::SpanContext {
fn from(sc: tracer::SpanContext) -> Self {
// TODO(Reviewer): Should this be try_from instead an propagate this error out of the WIT?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some of these conversions end up being fallible b/c we're parsing things that must conform to a format. Right now I handle this by just quelling the error and silently omitting the invalid data. This doesn't feel like a great user experience (even if it is documented in the WIT interface).

An alternative would be to make this try_from and then expose the error out through the WIT interface. This would make change:

  • start: func(name: string, options: option<start-options>) -> span; to start: func(name: string, options: option<start-options>) -> result<span>;
  • add-link: func(link: link); to add-link: func(link: link) -> result<()>;

I don't love the idea of exposing errors out of these functions just for the incorrect parsing of a span context. I'm not sure what the right choice is here.

@@ -1463,3 +1397,573 @@ route = "/..."
Ok(())
}
}

// TODO(Reviewer): How can I move this to a new file? I wasn't able to get the imports to work out.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't figure out how to put this module in another file, but still import things from the testcases module.

import tracer;
}

// TODO(Reviewer): Should we make this an experimental wasi package or a Spin package
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would suggest that we land WASI Observe as a release candidate wasi:[email protected]. I think it is the right choice to put it in the WASI namespace b/c WASI Observe is a phase 1 proposal.

If we're feeling conservative we could add this to a new http-trigger-experimental world where we make less support guarantees.

package fermyon:spin@2.0.0;

/// The full world of a guest targeting an http-trigger
world http-trigger {
  include platform;
  export wasi:http/incoming-handler@0.2.0;
}

/// Like `http-trigger`, but using WASI 0.2.0-rc-2023-10-18
world http-trigger-rc20231018 {
  include platform-rc20231018;
  export wasi:http/incoming-handler@0.2.0-rc-2023-10-18;
}

/// Like `http-trigger`, but experimental and with no support guarantees
world http-trigger-experimental {
  include platform-experimental;
  export wasi:http/incoming-handler@0.2.0;
}

/// The imports needed for a guest to run on a Spin host
world platform {
  include wasi:cli/imports@0.2.0;
  import wasi:http/outgoing-handler@0.2.0;
  import llm;
  import redis;
  import mqtt;
  import postgres;
  import mysql;
  import sqlite;
  import key-value;
  import variables;
}

/// Like `platform`, but using WASI 0.2.0-rc-2023-10-18
world platform-rc20231018 {
  include wasi:cli/reactor@0.2.0-rc-2023-10-18;
  import wasi:http/outgoing-handler@0.2.0-rc-2023-10-18;
  import llm;
  import redis;
  import mqtt;
  import postgres;
  import mysql;
  import sqlite;
  import key-value;
  import variables;
}

/// Like `platform`, but experimental and with no support guarantees
world platform-experimental {
  include platform;
  include wasi:observe/imports@0.2.2-rc2024-09-17;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

(This is probably a topic deserving of a SIP)

I think we should use WIT's feature gates for this, like so:

world http-trigger {
  include platform;
  export wasi:http/incoming-handler@0.2.0;
  @unstable(feature = wasi-observe)
  include wasi:observe@0.2.0-draft-2024-09-20/imports;
}

Additionally, unstable features should require runtime opt-in with something like a CLI flag along the lines of --enable-unstable-wit-features=wasi-observe,...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tschneidereit is there a reason you show it in the http-trigger world there instead of in platform?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes: sloppiness. It absolutely should be in platform

}

// TODO(Reviewer): Should we make this an experimental wasi package or a Spin package
// TODO(Reviewer): Would adding a metrics interface to this in a future version be a breaking change?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dicej does adding a new interface to a wold constitute a breaking change?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does not, no: additions to interfaces are semver-compatible. For now, that is not true for additions to:

  • function parameters
  • struct fields
  • tuple entries
  • variant and enum arms

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to clarify - it is not a breaking change from the perspective of the runtime, but it is from the perspective of the guest. In other words, if you add an import to a world, you can run old guests on an new runtime, but you cannot run new guests on an old runtime.

@calebschoepp calebschoepp marked this pull request as ready for review September 17, 2024 22:12
@calebschoepp calebschoepp changed the title feat(wasi-observe): WASI Observe factor (opentelemetry impl) feat(wasi-observe): WASI Observe factor implementation Sep 17, 2024
@calebschoepp
Copy link
Collaborator Author

calebschoepp commented Sep 18, 2024

TODO item for Caleb: It seems that this is not working with the Rust spin_sdk v3.0.1.

Signed-off-by: Caleb Schoepp <[email protected]>
@itowlson
Copy link
Contributor

@calebschoepp Not sure where you're at with this but a heads up that we now have a minty fresh new world for you to put the wasi:observe import into - see e.g. #2869 for an example

@calebschoepp
Copy link
Collaborator Author

@calebschoepp Not sure where you're at with this but a heads up that we now have a minty fresh new world for you to put the wasi:observe import into - see e.g. #2869 for an example

Thanks for leading the way on this @itowlson

@itowlson
Copy link
Contributor

@calebschoepp Just checking on status - is this something you're still planning to come back to, and if so is it close to completion - or is it parked for the foreseeable future? (I know you have a bunch of other stuff on, and completely understand that you've not had time to work on it - my intent is to ask about plans, not to nag!)

@calebschoepp
Copy link
Collaborator Author

@calebschoepp Just checking on status - is this something you're still planning to come back to, and if so is it close to completion - or is it parked for the foreseeable future? (I know you have a bunch of other stuff on, and completely understand that you've not had time to work on it - my intent is to ask about plans, not to nag!)

Thanks for checking in @itowlson. I definitely got very busy with other things and sort of just parked this — not sure when I'll have time to come back and finish this. But, I definitely want to land it at some point.

I would say the implementation is 80% of the way there. In fact this is more blocked by reaching consensus upstream in WASI Observe on what we want the API to be. Also there is some desire upstream to rename it from WASI Observe to WASI OTel b/c that is more accurate to what we're attempting to do with this WIT.

Does that give you enough detail? (I recognize I'm basically just saying idk when this will get done 🤣)

@itowlson
Copy link
Contributor

Yes, that's great context - thanks! "80% done until they remodel the API into a French bistro" gives me a solid sense of where this is at and the scope of the uncertainties around it!

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.

5 participants