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

feat: decrypted notifications analytics #286

Conversation

chris13524
Copy link
Member

@chris13524 chris13524 commented Nov 17, 2023

Description

Adds always_raw field to client info export, set to the value the client provided.

Adds always_raw to message export, which is set to the same value of the client, or null if the client cannot be found.

Makes encrypted and flags optional, since these are optionally provided from the relay now.

Adds tag field which is provided by the relay, and is technically optional during initial rollout.

Resolves #282

How Has This Been Tested?

Not tested

Due Diligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

@chris13524 chris13524 self-assigned this Nov 17, 2023
@arein arein added the accepted The issue has been accepted into the project label Nov 17, 2023
@chris13524 chris13524 changed the title feat: decrypted notifications/analytics feat: decrypted notifications analytics Nov 17, 2023
Copy link
Contributor

@geekbrother geekbrother left a comment

Choose a reason for hiding this comment

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

lgtm, except a comment.

pub flags: u32,
pub always_raw: Option<bool>,
pub tag: Option<u32>,
pub encrypted: Option<bool>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking. It's a bit confusing to have always_raw and encrypted. The value from encrypted comes from body.legacy maybe we should use legacy_encrypted? So it will not confuse the data team as well.

Copy link
Member Author

@chris13524 chris13524 Nov 20, 2023

Choose a reason for hiding this comment

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

We can't change the scheme since there is already data that's been exported using it. We'd have to wipe the data or adjust queries to exclude the old schema, which can be done but in this case doesn't seem necessary

@geekbrother geekbrother merged commit d1d4823 into feat/decrypted_notifs/update_message_payload_and_pass_raw Nov 21, 2023
6 checks passed
@geekbrother geekbrother deleted the feat/decrypted-notifications/analytics branch November 21, 2023 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted The issue has been accepted into the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants