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

MSC2477: User-defined ephemeral events in rooms #2477

Open
wants to merge 14 commits into
base: old_master
Choose a base branch
from

Conversation

ananace
Copy link
Contributor

@ananace ananace commented Mar 28, 2020

@ananace ananace changed the title MSC2476: User-defined ephemeral events in rooms MSC2477: User-defined ephemeral events in rooms Mar 28, 2020
@turt2live turt2live added proposal A matrix spec change proposal proposal-in-review labels Mar 28, 2020
@turt2live turt2live self-requested a review March 28, 2020 19:52
ananace and others added 2 commits March 30, 2020 09:38
Co-Authored-By: Sorunome <mail@sorunome.de>
As there's no similar requirement for PDUs, adding one for EDUs might be
a bit overkill.
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

overall this looks to be in the right direction!

proposals/2477-user-defined-ephemeral-events.md Outdated Show resolved Hide resolved
### Addition of an ephemeral event sending endpoint to the Client-Server API

The suggested addition to the CS API is the endpoint
`PUT /_matrix/client/r0/rooms/{roomId}/ephemeral/{eventType}/{txnId}`, which would act in an almost
Copy link
Member

Choose a reason for hiding this comment

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

worth noting that this limits custom EDUs to rooms compared to something like presence which is user based instead of room based.

Copy link
Contributor Author

@ananace ananace Aug 27, 2020

Choose a reason for hiding this comment

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

The second line in my proposal block already notes this, as I didn't think the usefulness of custom non-room EDUs merited the work of designing some whole new kind of permission system for sending them. Though perhaps a reference to user EDUs built on the Profile-as-a-Room MSC1769 could be added.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`PUT /_matrix/client/r0/rooms/{roomId}/ephemeral/{eventType}/{txnId}`, which would act in an almost
`PUT /_matrix/client/v1/rooms/{roomId}/ephemeral/{eventType}/{txnId}`, which would act in an almost

Following the new per-endpoint versioning spec.

### Extension of the ephemeral data received in /sync responses

Because the user-defined ephemeral events can't be aggregated and massaged by Synapse in a simple
manner, this then suggests instead adding a few more (**optional** but suggested for `m.*`,
Copy link
Member

Choose a reason for hiding this comment

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

"optional but suggested" is not really a state we can support. If we're bumping room versions, we should make it required.

It's also unclear how this would apply to something like presence which is not bound to a room and already duplicates this information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This proposal was by design only made for room-specific ephemeral events, so presence - as a non-room-bound EDU - shouldn't be affected. I'll try to make this more clear.

proposals/2477-user-defined-ephemeral-events.md Outdated Show resolved Hide resolved
proposals/2477-user-defined-ephemeral-events.md Outdated Show resolved Hide resolved
@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
Co-authored-by: Šimon Brandner <simon.bra.ag@gmail.com>
@turt2live
Copy link
Member

This could do with sanity review so an implementation can be written relatively safely.

@anoadragon453 anoadragon453 added the requires-room-version An idea which will require a bump in room version label Nov 16, 2021
@anoadragon453 anoadragon453 self-assigned this Nov 16, 2021
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

This is a really exciting feature, and I'd love to have it personally.

The below are my thoughts on all currently outstanding issues.

### Addition of an ephemeral event sending endpoint to the Client-Server API

The suggested addition to the CS API is the endpoint
`PUT /_matrix/client/r0/rooms/{roomId}/ephemeral/{eventType}/{txnId}`, which would act in an almost
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`PUT /_matrix/client/r0/rooms/{roomId}/ephemeral/{eventType}/{txnId}`, which would act in an almost
`PUT /_matrix/client/v1/rooms/{roomId}/ephemeral/{eventType}/{txnId}`, which would act in an almost

Following the new per-endpoint versioning spec.


Status code 400:

The user tried to send a `m.*` EDU, instead of using the type-specific endpoint
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we should make this off-limits to anything m.*. In fact, I wonder if we should go the opposite way and transition all room-based ephemeral messages to this endpoint - so read receipts and typing notifications.

At the moment, typing notifications are using:

PUT /_matrix/client/v3/rooms/{roomId}/typing/{userId}

{
  "timeout": 30000,
  "typing": true
}

and read markers are sent with:

POST /_matrix/client/v3/rooms/{roomId}/read_markers

{
  "m.fully_read": "$somewhere:example.org",
  "m.read": "$elsewhere:example.org"
}

The {userId} bit of the typing endpoint is especially useless, and could be cleaned up in the same move.

Presence and to-device messages would be excluded from this, as they are not bound to a room.

Copy link
Contributor

Choose a reason for hiding this comment

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

Read markers seem to be a bit of a special case, as they have an effect on the server to store something quasi-persistent, shouldn't read_markers stay out of this then? or what is the idea behind that?

Copy link
Contributor Author

@ananace ananace Nov 30, 2021

Choose a reason for hiding this comment

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

I wrote this MSC on the assumption that the custom EDUs would be a separate beast to the built-in EDUs, but I guess it could just as well be done to instead have the server handle well-defined EDUs in a certain manner. So that you - as a client dev/user - could just post m.typing or m.receipt/m.fully_read/m.read EDUs instead of calling the specific endpoints for them.

Copy link
Member

Choose a reason for hiding this comment

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

Read markers seem to be a bit of a special case, as they have an effect on the server to store something quasi-persistent, shouldn't read_markers stay out of this then? or what is the idea behind that?

Good point. m.fully_read does not fit this model, as its purpose is to store your read-up-to position, which lives in the current user's room account data. The linked endpoint just happens to allow optionally sending an m.read receipt in the same call.

Instead, we should be thinking about whether to replicate the behaviour of POST /_matrix/client/v3/rooms/{roomId}/receipt/{receiptType}/{eventId} with this then.

With that, I'm not so sure. They do fall under the banner of EDU, but a receipt is always tied to an event ID (e.g. I read up to this event ID). We'd need to include that event ID in the body of the request for m.read or for any other type of receipt. That can work, though I suppose it comes down to how much we'd like to differentiate receipts as their own thing, versus just another type of EDU.

Comment on lines 144 to 146
To reduce the complexity of the change, this proposal suggests to - for the time being - only limit
the user-defined types by these power levels changes. The default values for `m.*` specified here in
`ephemeral` would then only be expected to be informational in purpose.
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned earlier, any change to the auth rules requires a new room version. If we wanted to have these only apply to user-defined types initially, and then fully apply later, that would take two room versions.

Given rooms will need to be recreated anyways, it seems sensible to simply have homeservers pre-fill 0 for m.typing and m.fully_read entries when creating a room to preserve existing behaviour?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used m.receipt as the type when I was writing, as I wasn't sure about m.read (and m.fully_read). The resulting event is m.receipt, and the endpoint for sending the information is rooms/{roomId}/receipt. But the receipt type as defined in the content is m.read.
m.fully_read is in the spec handled in a separate endpoint, but includes a link to receipts as you can include m.read in the request to combine the two into a single request. Not sure how that would be combined into things.

For now I think I'm going to keep m.receipt as the general EDU type, since it's the type that - for now - is seen in an actual ephemeral event. (after all m.fully_read only ends up in the account data, and m.read are agglomerated into m.receipt events)

]
},
"type": "m.typing",
"room_id": "!636q39766251:example.com"
Copy link
Member

Choose a reason for hiding this comment

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

room_id isn't part of the spec for ephemeral event bodies down /sync.

Likewise below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The spec entry you linked includes an m.typing event in the example response for code 200, which does include room_id in the data;

# ...
    "join": {
      "!726s6s6q:example.com": {
        "account_data": {
          "events": [
            {
              "content": {
                "tags": {
                  "u.work": {
                    "order": 0.9
                  }
                }
              },
              "type": "m.tag"
            },
            {
              "content": {
                "custom_config_key": "custom_config_value"
              },
              "type": "org.example.custom.room.config"
            }
          ]
        },
        "ephemeral": {
          "events": [
            {
              "content": {
                "user_ids": [
                  "@alice:matrix.org",
                  "@bob:example.com"
                ]
              },
              "room_id": "!jEsUZKDJdhlrceRyVU:example.org",
              "type": "m.typing"
            }
          ]
        },
# ...

I think that's where that came from, since I tried to base my examples directly on the spec examples.

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks for pointing that out! I've made a PR to fix that, as I believe that's a spec bug: #3679.

proposals/2477-user-defined-ephemeral-events.md Outdated Show resolved Hide resolved
Comment on lines +328 to +331
This could cause some clients to break though, if they expect the well-defined objects to keep to
their specced forms. Additionally, it might be hard for the server to assign a correct sender and
timestamp to events if they are aggregated from multiple sources - e.g. typing notices and read
receipt lists.
Copy link
Member

Choose a reason for hiding this comment

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

This could cause some clients to break though, if they expect the well-defined objects to keep to
their specced forms.

If I understand correctly, this is referring to adding the sender and origin_server_ts fields to a JSON object. In the past I believe we've considered adding keys to JSON to not break backwards compatibility, as clients should just be pulling out the keys they need anyhow - and these events are not signed or otherwise wholly checked for integrity.

Additionally, it might be hard for the server to assign a correct sender and
timestamp to events if they are aggregated from multiple sources - e.g. typing notices and read
receipt lists.

This would have to be a problem that we solve anyways, regardless of backwards-compatibility periods.

I'm also not clear why it would be difficult to populate these fields for typing and read receipt lists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As one example on the second point; m.typing events are aggregated down from the source data into a single event with a list of user_ids that are currently typing. If they are to have correct timestamps and sender data attached to match the EDU type as defined in this MSC, then they'd have to be split apart into separate objects each with their own metadata. I'm not really sure if that's desirable.


Status code 400:

The user tried to send a `m.*` EDU, instead of using the type-specific endpoint
Copy link
Member

Choose a reason for hiding this comment

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

Read markers seem to be a bit of a special case, as they have an effect on the server to store something quasi-persistent, shouldn't read_markers stay out of this then? or what is the idea behind that?

Good point. m.fully_read does not fit this model, as its purpose is to store your read-up-to position, which lives in the current user's room account data. The linked endpoint just happens to allow optionally sending an m.read receipt in the same call.

Instead, we should be thinking about whether to replicate the behaviour of POST /_matrix/client/v3/rooms/{roomId}/receipt/{receiptType}/{eventId} with this then.

With that, I'm not so sure. They do fall under the banner of EDU, but a receipt is always tied to an event ID (e.g. I read up to this event ID). We'd need to include that event ID in the body of the request for m.read or for any other type of receipt. That can work, though I suppose it comes down to how much we'd like to differentiate receipts as their own thing, versus just another type of EDU.

proposals/2477-user-defined-ephemeral-events.md Outdated Show resolved Hide resolved
proposals/2477-user-defined-ephemeral-events.md Outdated Show resolved Hide resolved
proposals/2477-user-defined-ephemeral-events.md Outdated Show resolved Hide resolved
proposals/2477-user-defined-ephemeral-events.md Outdated Show resolved Hide resolved
Comment on lines 131 to 136
These new keys are to function in an identical manner to the already existing `events` and
`events_default` keys, with the assumed default value for `ephemeral_default` - if there is no
`ephemeral_default` in the `m.room.power_levels` event - being 50, while the default values for
`ephemeral` - if there is no `ephemeral` in the `m.room.power_levels` event - would consider all types
to be `ephemeral_default`, or 0 if there is no `m.room.power_levels` event - which would then not allow
any ephemeral events to be sent.
Copy link
Member

Choose a reason for hiding this comment

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

I found this paragraph a bit difficult to parse. Partly because it's all one sentence. How about:

These new keys are to function in an identical manner to the already existing events and
events_default keys. The default value for ephemeral_default is 50, while the default value for the
ephemeral dictionary would be {} or an empty mapping. This would imply that all ephemeral event types
would require a power level of at least ephemeral_default to send.

I think the last line about a missing m.room.power_levels state event is not necessary. According to the spec, rooms must contain an m.room.power_levels when created: https://spec.matrix.org/v1.2/client-server-api/#post_matrixclientv3createroom

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to rewrite it in a slightly different manner, should hopefully be more easily parsed now.

proposals/2477-user-defined-ephemeral-events.md Outdated Show resolved Hide resolved
proposals/2477-user-defined-ephemeral-events.md Outdated Show resolved Hide resolved
proposals/2477-user-defined-ephemeral-events.md Outdated Show resolved Hide resolved
proposals/2477-user-defined-ephemeral-events.md Outdated Show resolved Hide resolved
@YousefED
Copy link

YousefED commented Feb 9, 2022

Thanks for working on this! Definitely a promising feature.

Therefore I suggest that servers are only required to provide best-effort delivery, with the exact method in how they propagate EDUs - and store them - left up to implementation.

This triggered my attention. Performance-wise, I think servers should take into account that there can be many (orders of magnitude larger than regular events) ephemeral events sent for certain scenarios. This rules out any kind of temporary storage I think. For me at least, that would be a useful requirement (scenario I'm thinking of is to keep track of a user's mouse position and sharing that as ephemeral events). Do you think this should be supported, and if so, made explicit in the MSC?

@ananace
Copy link
Contributor Author

ananace commented Feb 9, 2022

I was debating offering a list of possible delivery guarantees as part of the MSC, but I feel that the necessary language, permissions, and special cases necessary for supporting such would end up rather complex - as these are entirely user-specified events which would basically require such a thing to be a per-event choice, which would mean a lot of cases for servers to consider.
Hence why I only suggest requiring a best-effort delivery (a.k.a. no guarantees whatsoever), and then leave it up to the server implementation to decide what memory/storage constraints it might consider in regards to doing anything above best-effort - e.g. for clients that don't have an active sync at that very moment.

Of the example scenarios I provide in the MSC itself, the live-location feature, would also result in a large amount of events if a large group of people are using it to try and meet up, or if it's used to track a fleet of commercial vessels. MSC3672 has appeared as a more fully-realized version of that particular example scenario now, and I'm curious to see how the discussion will evolve in regards to this.

the regular Matrix types as well, which would remove the need for optional fields - but could in
return impact the federation between servers, if they're built to only handle the exact requirements
of the spec.

Copy link
Member

@anoadragon453 anoadragon453 Apr 7, 2022

Choose a reason for hiding this comment

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

Unstable prefixes will need to be defined in order for an implementation of the MSC to be carried out. I suggest implementation replace:

  • ephemeral and ephemeral_default fields in the m.room.power_levels state event with org.matrix.msc2477.ephemeral and org.matrix.msc2477.ephemeral_default respectively.
  • the PUT /_matrix/client/v1/rooms/{roomId}/ephemeral/{eventType}/{txnId} endpoint with PUT /_matrix/client/unstable/org.matrix.msc2477/rooms/{roomId}/ephemeral/{eventType}/{txnId}.

And an experimental room version with name org.matrix.msc2477 should be used where this feature is enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And a whole lot later than it should've been, but I've added a section on an unstable prefix.

Wasn't sure about adding blurbs on when the m.room.power_levels keys should be allowed, but I figured that it'd a bit superfluous since the only method that will act on them is limited to the experimental room version.

### Addition of an ephemeral event sending endpoint to the Client-Server API

The addition to the CS API is the endpoint
`PUT /_matrix/client/v1/rooms/{roomId}/ephemeral/{eventType}/{txnId}`, which would act in an almost

Choose a reason for hiding this comment

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

Personal nit: wouldn't something like /send_ephemeral or /send/...?ephemeral=true make more sense? An endpoint just named "ephemeral" doesn't really indicate that it sends an event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel it makes more sense to fully separate ephemeral events under their own endpoint, since they're handled completely different from timeline events - which is what /send sends, as well as state events - which are sent by /state.
But an argument could definitely be made towards handling them like device-specific events, with a /sendEphemeral endpoint.


The current ephemeral event system in Matrix is built on a sort-of guaranteed delivery - albeit with
mutation/consolidation - of the events. This might not be desirable with custom ephemeral events, as
they could contain volumes of data that's not as easy to keep around for a guaranteed delivery.

Choose a reason for hiding this comment

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

I wonder if it might make more sense to remove the mutation/consolidation expectation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, replacing the built-in m.read/m.presence/etc with proper types that aren't munged by the server is indeed something that could be done, but I'm writing this MSC entirely towards the addition of user-defined EDUs and nothing else - leaving the already existing EDU types as they are.


As the messages in question are ephemeral, I think the only guarantee that should be required is that
all users that are online when the message is sent will receive it. Anything above that should be
commended, but not required.

Choose a reason for hiding this comment

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

How does one define an "online" user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to avoid putting any specific requirements/definitions on what the best-effort guarantee should mean, hence why I'm using some more vague terms like "online".
I originally wrote the best-effort definition to be for any user that has an active sync request, but it ended up feeling rather arbitrary (not to mention leaving open the question of what would happen to EDUs that arrive while the user's client is busy processing the sync response), and also didn't really match with some of the user stories I had in mind while writing the MSC.

If I were to write my own Matrix server, with custom EDUs, I'd probably consider a size and time gated buffer for them, where all user sessions with an active sync request are guaranteed to receive the EDU, and any user session that starts a new sync request within the time gate - or before the buffer rolls over - may also receive it.
But that's probably quite different from how other servers would do it.

Copy link
Member

Choose a reason for hiding this comment

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

When I was thinking about how to implement this a year or so ago, I had come up with the idea to let the sender of the EDU choose their delivery guarantee from a pre-defined set. The homeserver could either store it indefinitely until the receiver comes online (think to-device messages), or allow the receiver to miss it if they're not currently syncing (i.e. ephemeral live location events). In the latter case, the homeserver may only hold that data for 1m or so before expiring it. The same would apply to federation if the remote homeserver were down.

@Natureshadow
Copy link

For me at least, that would be a useful requirement (scenario I'm thinking of is to keep track of a user's mouse position and sharing that as ephemeral events). Do you think this should be supported, and if so, made explicit in the MSC?

Why would anyone store, or even deliver, all updates on this? Maybe it would make sens for EDUs to only transmit the last value per EDU type. So, a server could store the contents of an EDU type until it can deliver to the recipient, and overwrite the content for any subsequent updates on the EDU received from the sender.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal requires-room-version An idea which will require a bump in room version
Projects
None yet
Development

Successfully merging this pull request may close these issues.