Proposed WIT for tracing portion of wasi-observe #15

calebschoepp · 2024-09-05T20:23:04Z

This PR proposes WIT for what I think the tracing portion of wasi-observe should look like. Initial discussion of high level approaches was had here and this WIT is the outcome of that. I have about 50% of a working host implementation for this WIT in Spin so I'm pretty confident this is a workable approach implementation-wise.

Here are some of the design principles I followed while writing this WIT:

Closely follow the OpenTelemetry tracing specification to ease compatibility with that ecosystem.
But, not perfectly. I don't think wasi-observe should be 100% coupled to the OpenTelemetry ecosystem. For example here are just a few of the many things we're not following.
- "There MUST NOT be any API for creating a Span other than with a Tracer."
- "[The new span API] MUST NOT accept a Span or SpanContext as parent, only a full Context."
Liberally use option<T> rather than providing multiple versions of methods e.g. having start-implicit: static func(name: string) and start-with-options: static func(name: string, options: start-options) and ...

I'm happy to discuss the validity of these design principles and whether we want to take a different approach. They are not givens.

Signed-off-by: Caleb Schoepp <[email protected]>

lann · 2024-09-05T20:30:20Z

wit/traces.wit

+        /// Signals that the operation described by this span has now ended.
+        ///
+        /// If a timestamp is not provided then it is treated equivalent to passing the current time.
+        end: func(timestamp: option<datetime>);


Is dropping the resource equivalent to end(none)?

Yeah. That should probably be documented. I need to do some more thinking about the lifetimes of resources. B/c if we're forced to end a span when a resource is dropped then we might be breaking some of the OTel semantics.

I'd rather have a method called "abandon" instead.

Can you clarify what you'd want to have abandon be an alternative to?

lann · 2024-09-05T20:32:22Z

wit/traces.wit

+    }
+
+    /// A key-value pair describing an attribute.
+    record key-value {


Suggested change

record key-value {

record attribute {

🤷

I also feel 🤷 on this. The Rust and Go OTel implementations call this KeyValue. But, the spec talks about it as an attribute. I think all things considered I would lean to sticking with key-value. Especially given this line of the spec:

An Attribute is a key-value pair

lann · 2024-09-05T20:55:25Z

wit/traces.wit

+    /// Represents a unit of work or operation.
+    resource span {
+        /// Starts a new span with the given name, parent, and options.
+        start: static func(name: string, parent: span-parent, options: option<start-options>) -> span;


If parent is "almost always" implicit it could maybe move inside options? 🤷

I like this

calebschoepp · 2024-09-10T14:54:35Z

wit/traces.wit

+    /// The trace that this `span-context` belongs to.
+    type trace-id = tuple<u64, u64>;
+
+    /// The id of this `span-context`.
+    type span-id = u64;


One of the disadvantages of storing these types in u64's is that we need to be clear whether we're using little or big endian encodings. Other options here would be:

tuple<u8, u8, u8, u8, u8, u8, u8, u8,> — Pretty gross type.

list<u8> — Can't statically guarantee that it will contain the right number of bytes.

Use a fixed length array — Doesn't yet exist in WIT.

string (hexadecimal encoding) — Maybe this is a better option?

I would prefer it to be a string as well.

Use a fixed length array — Doesn't yet exist in WIT.

Might be worth a look at WebAssembly/component-model#384 — it sounds like fixed-length arrays may exist soon?

scb01 · 2024-09-11T14:54:19Z

wit/traces.wit

+        /// Whether this `span-context` was propagated from a remote parent.
+        is-remote: bool,
+        /// The `trace-state` for this `span-context`.
+        trace-state: string,


shouldn't trace-state should be of type trace-state that you've defined in line 152?

scb01 · 2024-09-11T14:56:18Z

wit/traces.wit

+        /// `link`'s for the new `span`.
+        links: option<list<link>>,
+        /// When the `span` should begin. If this is not provided it defaults to the current time.
+        timestamp: option<datetime>,


Here and in other places: I know that open telemetry has setting time as an option, but should we consider not supporting it? A side benefit would be that this wit wouldn't have a dependency on wasi::clocks...

I am curious to learn from others in the community about use cases and their opinions on whether this flexibility is needed.

I personally view the dependency on wasi::clocks as not a very large downside and think that it is worth supporting setting these timestamps. But, I'm also curious on what the sentiment of the community is.

scb01 · 2024-09-11T14:59:13Z

wit/traces.wit

+    /// Represents a unit of work or operation.
+    resource span {
+        /// Starts a new span with the given name, parent, and options.
+        start: static func(name: string, parent: span-parent, options: option<start-options>) -> span;


scb01 · 2024-09-11T15:23:29Z

wit/traces.wit

+        add-link: func(link: link);
+
+        /// Override the default `span` status, which is unset.
+        set-status: func(status: status);


nit: consider having status as an optional field in start-options and renaming this to update-status

I think the idea with status is that 90% of the time you should leave it unset. In the cases where you do set it, it can only be done later in the operation when you know its outcome. Source.

scb01 · 2024-09-12T20:32:39Z

wit/traces.wit

+    /// The trace that this `span-context` belongs to.
+    type trace-id = tuple<u64, u64>;
+
+    /// The id of this `span-context`.
+    type span-id = u64;


I would prefer it to be a string as well.

scb01 · 2024-09-12T20:52:08Z

wit/traces.wit

+        /// The `span`'s parent is the current active span. The current active span is the most recently created and non-closed span. If no spans have been started in the guest this may be a span in the host.
+        implicit,
+        /// The `span` is a root span and should have no parent.
+        is-root,


I'd like to discuss the implications of making the span-parent settable at start. Here is the way I understand it

Implicit means that the span will appear in a logical, expected place as a child of the previously executing span.

is-root will detach this span from the previous one and will likely be the parent of a net new trace tree?

span-context will make this a child of a different span, possibly belong to a different trace tree.

As of now, I find myself being the most uncomfortable about 3, as I'm unable to think of a usecase where this will apply.

You're understanding of the three options is correct. I largely designed it this way to follow the spec. However, I'm also starting to think that supporting options 2 and 3 is a mistake (at least for the first version of this WIT).

With option 2 I can sort of imagine a scenario where a user would want it. For example maybe they're running an app in Wasm that pulls work items off a queue. They might want the spans of pulling the work item to be part of the host trace — but then have the spans for actually doing the work be their own root span and thus separate trace.

With option 3 I can't imagine a way that it would be useful, but I can imagine lots and lots of ways that it could break things.

Overall I'm becoming skeptical that options 2 and 3 make sense in the context of a guest. We can always iterate so I'd prefer to start with something simple and would advocate we just remove the choice altogether and just always make it implicit.

Yeah, for option 2 an example was a scenario that I'd encountered previously, in which it was convenient to have the initiator of a set of tasks and the work done to fulfill per task be in separate traces. So, it has some utility.

That said, I am onboard with keeping the wit simple by only supporting implicit for now and evolving from there.

calebschoepp · 2024-09-18T19:35:34Z

@chrisdickinson @brooksmtownsend wondering if either of you have had a chance to look at this.

- Rename traces interface to tracer - Change some types - Move start to the interface rather than being a constructor - Remove abillity to specify arbitrary span context as parent Signed-off-by: Caleb Schoepp <[email protected]>

lann · 2024-09-20T13:14:19Z

wit/tracer.wit

+    }
+
+    /// Configuration for starting a `span`.
+    record start-options {


I'm wondering if this ought to be a resource to provide better forward compatibility for adding options.

Can you have fields on a resource or would all the data be stored in the host table?

No fields for resources; it would all be stored by the resource owner, which would typically be the host.

@brooksmtownsend what do you think about this?

lann · 2024-09-25T17:35:27Z

wit/tracer.wit

+    }
+
+    /// The `status` of a `span`.
+    variant status {


You could arguably represent this as an option<result<_, string>> 🤷

+1 for type status = option<result<_, string>>;

Interesting. One of the things I like about the variant is it's option correspond directly with the OTel terms that have specific semantic meanings.

Do y'all still think it is worth it using the more WIT native type status = option<result<_, string>>;

I don't feel strongly about this one.

I think I agree with @calebschoepp here, I like the direct tie to the OTEL meaning

lann · 2024-09-25T17:38:06Z

wit/tracer.wit

+        %float64(float64),
+        /// A signed 64 bit integer value.
+        %s64(s64),


I'd maybe name these float and int 🤷

Also is it still float64 or f64 ?

Using float64 is just an artifact of Spin being on an old wit-bindgen version.

fibonacci1729 · 2024-09-25T18:16:54Z

wit/tracer.wit

@@ -0,0 +1,151 @@
+interface tracer {


Could you throw some documentation on this interface?

What do you think about just calling this trace? wasi:observe/trace.start feels nice. Don't want to bikeshed on the name too hard at this stage so feel free to just ignore

I originally had it named something very similar to that — traces.wit.

I changed it to tracer.wit on the premise that the interface itself is acting as the Tracer. In the future if/when we support WIT templating then templating would act as the [TracerProvider](https://opentelemetry.io/docs/specs/otel/trace/api/#tracerprovider).

I imagine that we would also have a similar pattern for meter.wit and logger.wit.

brooksmtownsend

Hey @calebschoepp, fantastic job here! I really like the direct ties to the traces page from the otel docs. Feels like a great approach.

I had mostly nits and a couple of comments that I'm interested in and happy to talk sync in the next meeting too.

(p.s. thank you for the patience on the review here, I lost this in my inbox)

brooksmtownsend · 2024-09-30T21:39:38Z

wit/tracer.wit

+        /// The attribute name.
+        key: key,


nit: I generally prefer not using simple type aliases in WIT since it's just another mental lookup, thoughts on just making this a string?

I have the same thoughts later about trace-id / span-id etc etc but just to keep the comment spread simple I'll raise that here

key being type aliased was very much a result of me closely mirroring the OTel spec. Thinking about it some more I agree that we should just remove this alias and have key-value defined as:

/// A key-value pair describing an attribute. record key-value { /// The attribute name. key: string, /// The attribute value. value: value, }

As for trace-id / span-id, good catch. I also think that those should no longer be type aliased. Earlier they were some weird data structure like tuple<u64, u64> which made an alias more relevant.

/// Identifying trace information about a span that can be serialized and propagated. record span-context { /// The `trace-id` for this `span-context`. trace-id: string, /// The `span-id` for this `span-context`. span-id: string, ... }

That sounds good! Looks like I was missing a bit of history but glad it's landing in a simpler place 😄

brooksmtownsend · 2024-09-30T21:40:23Z

wit/tracer.wit

+        /// If a key already exists for an attribute of the Span it will be overwritten with the corresponding new value.
+        set-attributes: func(attributes: list<key-value>);
+
+        /// Adds an event with the provided name at the curent timestamp.


nit

Suggested change

/// Adds an event with the provided name at the curent timestamp.

/// Adds an event with the provided name at the current timestamp.

I'm a little surprised this made it past my VS Code spelling extension

wit/tracer.wit

brooksmtownsend · 2024-09-30T21:49:23Z

wit/tracer.wit

+    }
+
+    /// The `status` of a `span`.
+    variant status {


I think I agree with @calebschoepp here, I like the direct tie to the OTEL meaning

brooksmtownsend · 2024-09-30T21:52:49Z

wit/tracer.wit

@@ -0,0 +1,151 @@
+interface tracer {


What do you think about just calling this trace? wasi:observe/trace.start feels nice. Don't want to bikeshed on the name too hard at this stage so feel free to just ignore

calebschoepp requested review from chrisdickinson and brooksmtownsend September 5, 2024 20:23

feat(wit): Propose WIT for tracing

0f64b80

Signed-off-by: Caleb Schoepp <[email protected]>

calebschoepp force-pushed the wit-proposal branch from 410bdcf to 0f64b80 Compare September 5, 2024 20:28

lann reviewed Sep 5, 2024

View reviewed changes

calebschoepp commented Sep 10, 2024

View reviewed changes

scb01 reviewed Sep 12, 2024

View reviewed changes

This was referenced Sep 17, 2024

feat(wasi-observe): WASI Observe factor implementation fermyon/spin#2787

Open

Tracing WIT #12

Closed

Small changes

bf25635

- Rename traces interface to tracer - Change some types - Move start to the interface rather than being a constructor - Remove abillity to specify arbitrary span context as parent Signed-off-by: Caleb Schoepp <[email protected]>

lann reviewed Sep 20, 2024

View reviewed changes

lann reviewed Sep 25, 2024

View reviewed changes

fibonacci1729 reviewed Sep 25, 2024

View reviewed changes

brooksmtownsend reviewed Sep 30, 2024

View reviewed changes

	/// Adds an event with the provided name at the curent timestamp.
	/// Adds an event with the provided name at the current timestamp.

Proposed WIT for tracing portion of wasi-observe #15

Are you sure you want to change the base?

Proposed WIT for tracing portion of wasi-observe #15

Conversation

calebschoepp commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebschoepp commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lann Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brooksmtownsend left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs Sep 18, 2024 •

edited

Loading

lann Sep 25, 2024 •

edited

Loading