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

Simplifies wasi-messaging interface with feedback #24

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

Conversation

danbugs
Copy link
Collaborator

@danbugs danbugs commented Oct 4, 2024

This PR is a follow-up to #23 . Like it, this also incorporates various points of feedback from interface champions, users, and working group discussions. In addition, it also simplifies the wasi-messaging interface as per feedback in the prior PR.

The main changes to review are:

  • updated sections (e.g., introduction, goals, and portability) in the README.
  • removal of the consumer.wit.
  • refactor of messaging-guest as incoming-handler.
  • addition of request-reply.wit.
  • refactor of types.wit including creating concrete error types, and adding a message resource.

thomastaylor312 and others added 9 commits February 27, 2024 11:33
This makes several updates to the messaging interface. Initially the
README said that this wasn't going to support request/reply, but based
on my reading of the Kafka, NATS, MQTT, and SQS APIs, this is a fairly
common pattern. Another piece of evidence here is what I've seen as a
wasmCloud maintainer from our users. Request/reply is one of the more
common things we see with a messaging service. Please note that this
doesn't _require_ the use of a reply-to topic, just exposes it for use.

I also did a few other changes here. First is that I added the topic to
the message. This was common across all systems and is often used by code
to select the appropriate logic to perform. I also removed the format
field as this didn't seem to be a common parameter across various services.
We could definitely add a content-type member to this record in the future
if needed, but I think much of that can be passed via the metadata field.

There are other things I might suggest some changes to, but I want to think
on them some more and open some issues to discuss them first

Signed-off-by: Taylor Thomas <taylor@cosmonic.com>
This PR integrates various changes from talking to current users of
messaging in the community as well as conversations among the champions

Signed-off-by: Taylor Thomas <taylor@cosmonic.com>
I also deleted the examples.md for now until we settle on the interface.
It will be easier to add back in once we have some real world examples
to point at

Signed-off-by: Taylor Thomas <taylor@cosmonic.com>
Also removes extensions as a guest configuration option (for now)

Signed-off-by: Taylor Thomas <taylor@cosmonic.com>
In many of the interfaces out there right now, we've moved more towards
just calling these things config

Signed-off-by: Taylor Thomas <taylor@cosmonic.com>
Also removes the channel parameter I forgot to remove in a previous
commit

Signed-off-by: Taylor Thomas <taylor@cosmonic.com>
…ions

One of the uses of request-multi is to support a scatter/gather operation.
In these cases, you might not know how many requests you are going to
receive, so you can't set expected replies. Generally these wait until
timeout and then return the results. This commit adds the ability to
support all the different use cases for request-multi

Signed-off-by: Taylor Thomas <taylor@cosmonic.com>
After a whole bunch of feedback from the community, I realized we were
still trying to make this interface too much. So I dramatically paired
back the interface to be purely focused on message passing. Further
features like ack/nack, guaranteed delivery, and so on are now out of
scope (see the README for full details).

This was partly inspired by a discussion in the CNCF Wasm WG around this
interface. To be perfectly frank, the level I paired this down to is
essentially the same level of guarantees offered by the wasmCloud
[messaging interface](https://github.com/wasmCloud/messaging). The main
reason being is that there are people actually using that interface for
real applications (with real host implementations). If we can come to
agreement on a simpler interface, it will be easier to add in functionality
such as the things I stripped out in this commit.

Please let me know any feedback you have around this, focusing on whether
or not this covers at least the most basic scenarios

Signed-off-by: Taylor Thomas <taylor@cosmonic.com>
…d nits

Signed-off-by: danbugs <danilochiarlone@gmail.com>
Signed-off-by: danbugs <danilochiarlone@gmail.com>
.idea/.gitignore Outdated Show resolved Hide resolved
Signed-off-by: danbugs <danilochiarlone@gmail.com>
wit/types.wit Show resolved Hide resolved
Copy link

@yordis yordis left a comment

Choose a reason for hiding this comment

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

They are multiple files with No newline at end of file not sure if that should be fix, ideal .editorconfig and things like that would help with IDEs doing the right thing

wit/types.wit Outdated Show resolved Hide resolved
wit/request-reply.wit Outdated Show resolved Hide resolved
Signed-off-by: danbugs <danilochiarlone@gmail.com>
.gitignore Outdated Show resolved Hide resolved
wit/messaging.wit Show resolved Hide resolved
wit/producer.wit Outdated

send: func(c: client, ch: channel, m: list<message>) -> result<_, error>;
/// Sends the message using the given client.
send: func(c: borrow<client>, m: message) -> result<_, error>;
Copy link

Choose a reason for hiding this comment

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

The topic should be an argument of the send function. I would also make the argument names more revealing:

func(client: borrow<client>, topic: topic, message: message)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed making the argument names more revealing across all functions ✅ —as for the topic name being part of the parameter, what if we then have a discrepancy between the message's topic and the provided topic? I feel like this adds ambiguity for implementations that could hurt the portability of the interface.

Copy link

Choose a reason for hiding this comment

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

I'd argue for moving the topic from the message to the send function from two different sides: the domain modeling and the implementation side.

On the domain model side, a message should be independent from the (possibly many) topics it is published on. I should be able to send the message "Fire!" to floors.first, floors.second, and floors.third without changing or cloning it.

On the implementation side, backend systems have different semantics:

Kafka most closely resembles the original intent but introduces a ProducerRecord that is very closely tied to the semantics of the producer (in contrast to a message):

ProducerRecord<String, String> record = new ProducerRecord<>("floors.first", "Fire!");
producer.send(record);

RabbitMQ (routing_key as topic):

channel.basic_publish(exchange='alarms',
                      routing_key='floors.first',
                      body=message)

NATS (Rust client; subject = topic)

let subject = "floors.first";
let data = Bytes::from("Fire!");
client.publish(subject, data.clone()).await?;

Copy link

Choose a reason for hiding this comment

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

without changing or cloning it.

I think that is where the disconnection is happening, should they all be the message? Or should you have n message, one for each send if you know the topic is different?

wit/types.wit Outdated
/// The topic/subject/channel this message was received or should be sent on
topic: func() -> string;
/// Set the topic/subject/channel this message should be sent on
set-topic: func(topic: string);
Copy link

Choose a reason for hiding this comment

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

The topic should not be required for the purpose of publishing or sending it. It can be informational (more like metadata).

While the notion of a single topic or subject maps to some messaging systems very well (e.g. NATS), it is not a good fit for others (e.g. RabbitMQ).

Copy link

Choose a reason for hiding this comment

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

I needed to clarify this, but I never said anything; I self-reply. Could it be an ambiguous language problem?

Is the intent that such a Message is the very last data structure in the component, so you must say where it is going at that point?

Copy link

Choose a reason for hiding this comment

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

Just to clarify: I am advocating to add the topic to the send function, instead of the message. I believe that the topic is part of the publishing process, and not the message. The message should be able to stand by itself.

That said, keeping the topic in the message makes sense in the context of provenance.

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'll defer this to @thomastaylor312 —do you want to elaborate a bit more on the reason behind the change of having topic be a part of message instead of an argument to send?

Copy link

@yordis yordis Oct 4, 2024

Choose a reason for hiding this comment

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

I thought that,
To make a point, I see message as a send-command single argument object, so such a command object has all the information for a given use case (send) to be done.

I know it is confusing because message is such a generic word, and the perspective of such an object matters; I am with the topic to be part of it because such an object is already the last object to be sent as a command message.

wit/guest.wit Outdated Show resolved Hide resolved
…metadata method, added new-line at the end of files, improved argument names, removed .gitignore, improved documentation, changed get-subscriptions function name

Signed-off-by: danbugs <danilochiarlone@gmail.com>
Comment on lines +46 to +47
/// Add a new key-value pair to the metadata, overwriting any existing value for the same key
add-metadata: func(key: string, value: string);
Copy link

@yordis yordis Oct 4, 2024

Choose a reason for hiding this comment

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

Suggested change
/// Add a new key-value pair to the metadata, overwriting any existing value for the same key
add-metadata: func(key: string, value: string);
/// Add a new key-value pair to the metadata, overwriting any existing value for the same key
add-metadata: func(key: string, value: string);
/// Set the metadata
set-metadata: func(meta: metadata);

@danbugs, although, yes, we should keep remove-metadata. I was thinking about setting all the metadata when I do not know the keys or do not want to list the metadata and remove them individually.
Primarily thinking about relaying components that can not proxy the metadata at all

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.

6 participants