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

MSC2389: Toward the EDU-to-PDU transition: Typing. #2389

Closed

Conversation

tulir
Copy link
Member

@tulir tulir commented Dec 18, 2019

Author: @jevolk

Rendered

Signed-off-by: Jason Volk <jason@zemos.net>
@turt2live turt2live added proposal A matrix spec change proposal proposal-in-review labels Dec 18, 2019
Copy link
Contributor

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

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

Although EDUs are not by any means perfect, I do not believe that switching typing notifications to PDUs is the right approach.

First is that, once a message has been sent, the typing notification becomes irrelevant. By making it a PDU, we can’t just “forget” the typing notification, adding potentially lots of dead weight into the room history that probably still needs to be synced to and processed by clients (which might already be mobile, on slow data connections or constrained in other ways). As PDUs would become woven into the DAG of a room, it becomes a lot harder to filter out the noise and still be able to verify the predecessors of later events correctly.

Secondly, it opens up a new abuse vector by allowing room history to be significantly inflated with lots of now-useless events, by sending lots of typing notifications but no follow-up messages, that effectively are invisible to the average user after the fact. At least if you spam an active room with actual messages or redactions, someone will notice (and hopefully take action) quite quickly. I’m not sure how a room moderator would be expected to notice a flood of expired typing notifications.

Finally, servers would still have to store and replicate all of these additional events and those operations aren’t free either.

@yangm97
Copy link

yangm97 commented Jan 13, 2020

By making it a PDU, we can’t just “forget” the typing notification

I think there's work to make that sentence become untrue. #2228 seems to implement some of the behaviour described here, for instance.

@turt2live
Copy link
Member

Please make all comments in threads so people can reply without the use of quotes, thanks.

Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

There isn't much to go on why we should actually make this change. As it stands, typing notifications work perfectly fine in Matrix and haven't become a serious bottleneck compared to things like presence.

I suspect this would be more convincing if there was more elaboration and genuine examples of the benefits of typing-as-PDUs

PDU's and to be easily discarded afterward. In practice, typing events are a very
small part of the overall loads on implementations within Matrix. Consider in situ,
typing events repel each other because users actually avoid typing at the same
time. The result is that if typing indications were a part of the room timeline
Copy link
Contributor

Choose a reason for hiding this comment

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

because users actually avoid typing at the same time.

If typing notifications become PDUs, they will naturally become slower than the current EDU system. Notifications will take longer to reach each destination simply because they will require more computation to authenticate and embed into the DAG. If this takes longer, there is scope for users to "miss" typing notifications, because they take so long to arrive at the clients of other users, and the "repel effect" is lessened.

As an aside, I don't think there is a huge amount of proof that typing notifications do repel each other, and that simply might be wishful thinking. In some cases certainly, but it's not the rule.

Copy link

Choose a reason for hiding this comment

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

If typing notifications become PDUs, they will naturally become slower than the current EDU system.

Why?

Copy link
Contributor

Choose a reason for hiding this comment

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

Notifications will take longer to reach each destination simply because they will require more computation to authenticate and embed into the DAG

Copy link

Choose a reason for hiding this comment

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

That doesn't answer my question but maybe we should move performance concerns into #2390 as that would cover other EDU-to-PDU MSCs concerns in one place.

small part of the overall loads on implementations within Matrix. Consider in situ,
typing events repel each other because users actually avoid typing at the same
time. The result is that if typing indications were a part of the room timeline
they would not overload it by any means; we contend that the timeline would even
Copy link
Contributor

Choose a reason for hiding this comment

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

You should perhaps also expand upon "overload". Overload what? Overload how?

typing events repel each other because users actually avoid typing at the same
time. The result is that if typing indications were a part of the room timeline
they would not overload it by any means; we contend that the timeline would even
be enriched by their presence.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does having typing notifications in the timeline enrich it. Is there a crying need for typing notifications to appear in the DAG, that isn't solved by the current solutions?

@@ -0,0 +1,40 @@
# MSC2389 - Typing (keyboard-input) EDU-as-a-PDU
Copy link
Member

Choose a reason for hiding this comment

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

I think this MSC is predicated on the assumption that #2390 is merged and EDUs have been removed, and so there is nowhere else to put typing notifications than the DAG.

Considering the proposal independently of #2390: just because it's possible to store transient data like typing notifications in the timeline of a room, doesn't mean that there's any value in doing so. The only use-case I can think of would be to support 'time-travel' action replay of a room - but it's very unclear on how valuable this is, other than as a gimmick. It's certainly unclear that the value justifies the additional serverside load of DAG processing & persistence.

In an analogy to git: sure, you could go and persist audit log of transient and semantically insignificant operations (e.g. pulls, fetches, blames, diffs, logs) in the reflog of your repository. But in practice, you want to keep that data structure (and the associated data management overhead) for your actual code instead. There's no point in clogging it up with stuff which is irrelevant for the record of the structure of your code - or in this instance, conversation.

You could try to argue it both ways, but in practice this is a fairly fundamental design decision for the project: whether room timelines for IM are intended to be a full record of every interaction with the room, or whether you try to keep them lean and lightweight and don't clutter them up with irrelevant stuff. We chose the latter, and I'm not seeing a good reason to change that (assuming that EDUs are a thing).

@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 per #2389 (review): just because it's possible to store transient data like typing notifications in the timeline of a room, doesn't mean that there's any value in doing so.

@mscbot fcp close

@mscbot
Copy link
Collaborator

mscbot commented Apr 20, 2020

Team member @ara4n has proposed to close this. The next step is review by the rest of the tagged people:

Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for information about what commands tagged team members can give me.

@mscbot mscbot added disposition-close proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Apr 20, 2020
@mscbot
Copy link
Collaborator

mscbot commented May 12, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@mscbot mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels May 12, 2020
@mscbot
Copy link
Collaborator

mscbot commented May 17, 2020

The final comment period, with a disposition to close, as per the review above, is now complete.

@mscbot mscbot closed this May 17, 2020
@mscbot mscbot added finished-final-comment-period and removed disposition-close final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels May 17, 2020
@richvdh richvdh added obsolete A proposal which has been overtaken by other proposals rejected A proposal which has been rejected for inclusion in the spec labels Apr 26, 2022
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 obsolete A proposal which has been overtaken by other proposals proposal A matrix spec change proposal rejected A proposal which has been rejected for inclusion in the spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants