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

MSC2388: Toward the EDU-to-PDU transition: Read Receipts. #2388

Open
wants to merge 1 commit into
base: old_master
Choose a base branch
from

Conversation

tulir
Copy link
Member

@tulir tulir commented Dec 18, 2019

Author: @jevolk

Rendered

@turt2live turt2live added proposal A matrix spec change proposal proposal-in-review labels Dec 18, 2019
@Sorunome
Copy link
Contributor

It seems like a rather bad idea to have read receipts in the timeline.

Comment on lines +22 to +23
We specify the Event type m.receipt. Within the content of this event, an m.relates_to with a
rel_type of m.read provides and replaces the functionality of the existing receipt EDU.
Copy link

Choose a reason for hiding this comment

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

Speaking of replacing stuff, I suppose this proposal could describe that previous m.read events should be replaced by the latest versions, using the improved redactions, which I believe would allow servers/clients to skip/not store forever previous versions.

Copy link
Member

@uhoreg uhoreg Jan 15, 2020

Choose a reason for hiding this comment

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

What improved redactions? I'm not aware of any proposal to change redactions, other than adding a mass-redactions API, which AFAIK doesn't change how redactions work.

Copy link

Choose a reason for hiding this comment

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

#447 and #1763 come to my mind.

Copy link
Member

Choose a reason for hiding this comment

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

#1763 only deals with time-limited messages, which would not work well for read receipts; read receipts "expire" when replaced by another one, rather than after a certain amount of time. This would be more applicable to typing notifications, though I suspect that the servers would still need to keep a bunch of information about the event, so that the DAG doesn't have random holes in it that would prevent the room from working correctly.

@yangm97
Copy link

yangm97 commented Jan 14, 2020

It seems like a rather bad idea to have read receipts in the timeline.

Care to elaborate? We have avatar changes registered on every room a user participates, this seems much more pertinent to the room than that.

@turt2live
Copy link
Member

Please use threads on the diff so people can reply, thank you.

### Solution

We specify the Event type m.receipt. Within the content of this event, an m.relates_to with a
rel_type of m.read provides and replaces the functionality of the existing receipt EDU.
Copy link
Contributor

Choose a reason for hiding this comment

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

Care to elaborate? We have avatar changes registered on every room a user participates, this seems much more pertinent to the room than that.

(replying in a thread for easier future replies)

m.read doesn't seem like something you'd want in your timeline for permanent storage. It is irrelevant in the future when which user read what. So storing this information permanently is an unnecessary overhead, which could cause problems down the line, especially with clients in e.g. embedded enviroments which can't handle that many events.

Copy link

Choose a reason for hiding this comment

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

Using PDUs doesn't imply in permanent storage, per se. See my review above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Destructing events for m.read also seems like a bad idea as, if you e.g. don't log in for a week, suddenly all your read indicators are off.

In sorus opinion sticking with EDUs seems like the best option here.

Copy link

Choose a reason for hiding this comment

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

Destructing events for m.read also seems like a bad idea as, if you e.g. don't log in for a week, suddenly all your read indicators are off.

Assuming self destructing events, correct? What if instead of self destructing read indicators simply replace the previous, regardless of which one your server/client has currently resolved.

In sorus opinion sticking with EDUs seems like the best option here.

It seems to me that improved redactions made EDUs redundant, which is why there's some movement to push that away from the protocol.

Copy link
Contributor

Choose a reason for hiding this comment

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

Assuming self destructing events, correct? What if instead of self destructing read indicators simply replace the previous, regardless of which one your server/client has currently resolved.

Sounds like you are trying to re-create EDUs here, so why not take the existing model that works?

Copy link
Member

Choose a reason for hiding this comment

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

It does but I don't think it stores those receipts in a DAG - too much of a hassle for a centralised service. I'm also really unsure how many people actually look at the list of message readers (I only did it out of curiosity and never needed it). And the current MSC doesn't describe such new semantics. In fact, in doesn't really describe any changes in the semantics at all so I struggle to understand how such new receipts would work end-to-end.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I mark a room as read, I usually make sure, that I have at least read all the mentions. So while read receipts aren't an exact way to see if someone read the message, you can at least use them to ping someone again, if they sent a read receipt but didn't act on the message in the way you expected. So I think read receipts are good enough for that, if they are actually persisted (and don't just get replaced on the next sync, since I may want to check, if the person actually marked the message as read a few hours ago and not just a few seconds ago).

Copy link
Member

Choose a reason for hiding this comment

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

I can buy that in the context of a, say, a direct chat, or a very small (<10 members) room; but in that case (specifically for messages you want to track attention on) you might equally just ask the person whether they've seen the message and what they can say to it. My perception is that this is a fairly corner case (How often did you really miss such function?) and it doesn't justify a rather sweeping change; and the MSC doesn't really elaborate neither on use cases, nor on consequences of such change.

To rephrase: there might be merit in such change but from the text of the MSC it's terribly hard to see any because it doesn't mention neither use cases, nor consequences of the change; neither for the spec texts (you cannot just name an event a PDU, you have to define a new schema), nor for clients and servers.

Choose a reason for hiding this comment

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

I look at read receipts in WhatsApp all the time. Like I'm sending out a link to a jitsi call and I want to know whether people didn't read it or just don't want to join.
It might be useful to have a distinction between small groupchats and large public channels, because the semantics are very different for a lot of things.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'd probably agree that's a good case for more persistent read receipts but not for every message and, probably, not visible for everyone (not in "large public channels", one they become a thing in its own right in Matrix). Somewhat resembles an e-mail with a request for read confirmation.


### Problem

In practice, implementations treat read-receipt EDU's no different than other PDU messages. Upon
Copy link
Member

Choose a reason for hiding this comment

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

read receipts are (currently) treated fundamentally differently; you only have to track the most recent place that a user has read up to (or possibly in future, the ranges of events that they've read over, but it's still aggregated data you end up persisting). you don't care about modelling the full history of precisely when they read up to which point.

My same concerns apply as for https://github.com/matrix-org/matrix-doc/pull/2389/files#r366632795 - we simply don't want to clog up the DAG with irrelevant data.

This could make sense if EDUs didn't exist... but they do, and for good reason.

Copy link

Choose a reason for hiding this comment

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

we simply don't want to clog up the DAG with irrelevant data

What do you think of #2388 (comment) ?

Copy link

Choose a reason for hiding this comment

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

It just came to me that you may, in fact, want to store read receipts for every message, forever, in order to achieve reliable and verifiable auditorium room (equivalent of telegram channels) view counters.

### Problem

In practice, implementations treat read-receipt EDU's no different than other PDU messages. Upon
reception they must be persisted for effective client synchronization. Implementations also employ
Copy link
Member

Choose a reason for hiding this comment

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

That's an inaccurate statement. Notably, Quotient doesn't persist read receipts - it keeps them in memory but as soon as it's restarted, it loads a new map of read receipts from the server, not caring at all (why would it?) about the track record of read receipts.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's probably referring to the server. As you just said, you rely on the server to persist the "ephemeral" events (since it's obviously unacceptable from an UX PoV for read receipts to disappear when the client is restarted).

Copy link
Member

Choose a reason for hiding this comment

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

Well, that's still inaccurate even for server-side implementations, since persistence does not imply adding things to DAG.

### Solution

We specify the Event type m.receipt. Within the content of this event, an m.relates_to with a
rel_type of m.read provides and replaces the functionality of the existing receipt EDU.
Copy link
Member

Choose a reason for hiding this comment

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

The way read receipts are implemented now, be they PDUs or EDUs, you cannot reliably tell if a user has read all messages between two states of a given user's read receipt; there's a good chance that they've read some messages before the read receipt but that's really it. A very prominent example: if a person marks all messages as read that doesn't mean they actually have read them, despite the fact that the very next message they actually seen and possibly read will carry a read receipt. If you need a reliable confirmation of reading (e.g., to have a user formally confirm that they have read the message, it should be a separate event type (obviously a PDU, very likely in m.relation to the message event that's been read).

@turt2live turt2live added the kind:feature MSC for not-core and not-maintenance stuff label Apr 20, 2020
@ara4n
Copy link
Member

ara4n commented Apr 20, 2020

As the author says in the MSC, read receipts are currently special cased because we expect an order of magnitude more of them than messages themselves on average. Winding back that optimisation without good reason seems undesirable.

From the comments, there look to be two genuine use cases here though:

  • The ability to send arbitrary EDUs
  • The ability to track read state on a per message basis.

I suggest someone opens a new MSC for sending arbitrary EDUs. Meanwhile, I suggest we leave this one open to cover "track read state on a per message basis", although of all the features missing from Matrix today, I'm not convinced that's one that's holding back uptake of the protocol.

@turt2live
Copy link
Member

@ara4n Custom ephemeral events has an MSC already: #2477

@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
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants