Skip to content

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 richvdh and stefanceriu and removed request for a team December 9, 2024 16:46
@codecov
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

@andybalaam andybalaam force-pushed the andybalaam/distinguish-historical-utds branch from c8658d0 to ebbde1a Compare December 11, 2024 16:36
poljar
poljar previously requested changes Dec 12, 2024
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.

6 participants