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

Provide the reason why an event was an expected UTD #4384

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

andybalaam
Copy link
Member

@andybalaam andybalaam commented Dec 6, 2024

Part of element-hq/element-meta#2638

Distinguish between 3 cases where we can't find keys for an old message:

  • There is no backup -> HistoricalMessageAndBackupIsDisabled
  • Backup is not working and we have not verified our device -> HistoricalMessageAndDeviceIsUnverified
  • Something else -> Unknown

@andybalaam andybalaam changed the title feat(crypto) Provide a method to check whether server backup exists w… Provide the reason why an event was an expected UTD Dec 9, 2024
@andybalaam andybalaam force-pushed the andybalaam/distinguish-historical-utds branch 5 times, most recently from 9df9c0b to 5575637 Compare December 9, 2024 16:45
@andybalaam andybalaam marked this pull request as ready for review December 9, 2024 16:46
@andybalaam andybalaam requested review from a team as code owners December 9, 2024 16:46
@andybalaam andybalaam requested review from stefanceriu and richvdh and removed request for a team December 9, 2024 16:46
Copy link

codecov bot commented Dec 9, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.28%. Comparing base (150d9e4) to head (dca0482).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/matrix-sdk/src/room/mod.rs 75.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4384   +/-   ##
=======================================
  Coverage   85.28%   85.28%           
=======================================
  Files         282      282           
  Lines       31196    31207   +11     
=======================================
+ Hits        26605    26615   +10     
- Misses       4591     4592    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@stefanceriu stefanceriu left a comment

Choose a reason for hiding this comment

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

Super nice and clear, lgtm! 👏

Copy link
Member

@BillCarsonFr BillCarsonFr left a comment

Choose a reason for hiding this comment

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

I think we need a 3rd variant to stay consistent with web, or we need to update web to be consistent with EX

crates/matrix-sdk-crypto/src/types/events/utd_cause.rs Outdated Show resolved Hide resolved
@andybalaam andybalaam force-pushed the andybalaam/distinguish-historical-utds branch from c8658d0 to ebbde1a Compare December 11, 2024 16:36
Copy link
Member

@BillCarsonFr BillCarsonFr left a comment

Choose a reason for hiding this comment

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

I think it's an improvement but I'd like to have some follow up tasks.

For example we fallback to Unknwown in determine_historical. It's a bit annoying because we report Unknwown in posthog as OlmKeysNotSent which sounds bad. This key was never sent. So maybe we need a BackupUnknown that would be mapped to something else in posthog (or we check the event age before reporting in posthog)

And just another comment, I though that we were caching the exists_on_server() but I don't see that in this PR?
Are we going to do a server query at ~every UTD report?

@andybalaam
Copy link
Member Author

And just another comment, I though that we were caching the exists_on_server() but I don't see that in this PR? Are we going to do a server query at ~every UTD report?

Thank you for checking. Yes, that was done in #4356 so our call to exists_on_server in this PR uses the cached value where available.

@andybalaam andybalaam dismissed poljar’s stale review December 13, 2024 10:30

Changes addressed, thanks!

@andybalaam andybalaam force-pushed the andybalaam/distinguish-historical-utds branch from 68cc73e to dca0482 Compare December 13, 2024 10:31
@andybalaam andybalaam enabled auto-merge (rebase) December 13, 2024 10:31
@andybalaam andybalaam merged commit 6dcefe4 into main Dec 13, 2024
39 checks passed
@andybalaam andybalaam deleted the andybalaam/distinguish-historical-utds branch December 13, 2024 10:46
@andybalaam
Copy link
Member Author

I think it's an improvement but I'd like to have some follow up tasks.

For example we fallback to Unknwown in determine_historical. It's a bit annoying because we report Unknwown in posthog as OlmKeysNotSent which sounds bad. This key was never sent. So maybe we need a BackupUnknown that would be mapped to something else in posthog (or we check the event age before reporting in posthog)

Follow-up task is here: element-hq/element-meta#2666

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.

5 participants