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

Native Notifications #1428

Merged
merged 95 commits into from
Aug 23, 2023
Merged

Native Notifications #1428

merged 95 commits into from
Aug 23, 2023

Conversation

ansh
Copy link
Contributor

@ansh ansh commented Aug 3, 2023

Types of notifications handled

  • like
  • comment/reply
  • quote
  • repost
  • follow
  • custom feed like

Misc items

  • Separate AppView and PDS concerns
  • Move notification sending out of processor.ts into indexing service
  • Duplicate code in AppView
  • Add tests
  • Rate limiting and queueing

Future:

  • Show image previews in notification (need to do some CDN resizing/compression stuff for this) -- not very important
  • Handle notifs for web (gorush does not support web notifs)

@@ -230,6 +233,17 @@ export class RecordProcessor<T, S> {
notifs = this.params.notifsForInsert(op.inserted)
}
for (const chunk of chunkArray(notifs, 500)) {
if (this.notifServer) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems nicer to do this out in the indexing service instead of within the processor which I view as a "raw record -> DB indexes" translator 🤔

any reason to prefer it here?

Copy link
Collaborator

@devinivy devinivy Aug 22, 2023

Choose a reason for hiding this comment

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

I put it here in order mostly to make use of the existing code paths for notification handling, make it simpler to answer "where are notifications sent?", ensure the same logic was being applied no matter the destination of the notifications. Previously notifications that may be generated from a deletion weren't being handled, and the added notifsForInsert() calls down in insertRecord()/updateRecord() felt a bit leaky/special-cased to me.

I do see what you're saying, though. I may just have a slightly different perspective on it—that the destination of the notifications (in the db or off to some push service) isn't all that significant. And that having one code path for notifications is useful.

My guess is that we could get the best of both worlds with a little refactor! But now might not be the time for it. Happy to switch it back to the way it was if you think that's the move 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

nah that's cool I dig the justification 👍

if we end up maturing this system, that might be a good time to move it around a bit

const allUris = [...subjectUris, ...recordUris]

// gather potential display data for notifications in batch
const [authors, posts] = await Promise.all([
Copy link
Collaborator

Choose a reason for hiding this comment

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

i still wonder if we can avoid most of these db hits by passing the record into the notification service?

i will see - these bulk gets give a decent justification for processing notifs in batches

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe that does help with the post record lookup for mentions and replies, which do show the text from the post that was just indexed. The author display name lookups for all notifs and post record lookups for likes, quotes, reposts will still need to happen at some point. The changes I made here tried to batch work together into two queries rather than a few queries per notification. Batching more notifications together as you're suggesting will probably be what gives the biggest win here.

@devinivy devinivy merged commit eae682f into main Aug 23, 2023
@devinivy devinivy deleted the ansh/native-notifications branch August 23, 2023 22:37
@devinivy devinivy mentioned this pull request Sep 13, 2023
mloar pushed a commit to mloar/atproto that referenced this pull request Sep 26, 2023
* pseudocode for sending notification to user

* add notification push token table

* lexicon codegen

* `pds` and `api` codegen

* update lexicon

* add simple function to `putNotificationPushToken` into `notification_push_token` table

* reorgnaize imports

* add unspecced `putNotificationPushToken` to pds

* add `notification-push-tokens` table to PDS

* check if token exists before adding it to db

* add endpoint and appId to PDS table

* setup notification server

* fix logic for inserting token into db

* fix NotificationServer methods by making them static

* fix merge conflicts

* add comments on how sending ntoifications work

* remove dead code

* move notifServer to AppContext

* refactor code to handle notif sending in `Indexer` service

* add additional data when sending notifs

* clean up code

* move notif logic to indexer

* add `appId` and `endpoint` optional params to putNotificationPushToken

* clone notification code to AppView

* add endpoint to register push token with app view

* disable pds and enable app view notification server

* clean up code

* simply logic to check if a token already exists

* remove NotificationServer from PDS

* remove notification-push-token table from PDS

* remove `putNotificationPushToken` endpoint

* clean up code

* let `axios` throw error if `gorush` has an error

* let `kysely` throw error if notif cannot be registered by client

* rename `registerPushNotificationEndpoint` to `registerPushNotifications`

* delete `putNotificationPushToken` from AppView

* rename putPushNotificationToken to registerPushNotification

* remove dead notification code from pds

* remove sanitizeDisplayName from NotificationSever

* move `pushNotificationEndpoint` to config

* temp add `pushNotificationEndpoint` to dev-env setup

* remove example test from feed-generation.test.ts

* add test for registerPushNotification and clean up error handling

* move notifications test to its own file

* add test for NotificationServer to check if tokens are retrieved correctly

* remove unused functions from NotificationServer

* add additional tests for NotificationServer

* add return type to getNotificationDisplayAttributes function

* remove unnecessary comments

* remove dead NotificationServer code from PDS

* clean up code to prepareNotifsToSend

* put sending notifications as part of the backgroundQueue

* log instead of throwing error if notification attributes don't exist

* remove logs

* add more tests to `notification-server.test.ts`

* show replied with text for reply notifs

* better error handling when sending notifications via backgroundQueue

* add rate limit and batching to sending notifications

* add comments to NotificationServer

* merge with main

* use redis for rate limiting instead of normal rate limits

* move `notificationBatchSize` into config

* usePrimary db in test

* hoist push notif migration to present, update model to remove endpoint

* update push notif lexicon

* pare down lex for unregistering push

* helpers for working with service endpoints from did doc

* service-authed register/unregister push notifs

* add well-known endpoint to appview

* update bsky notif service tests

* fix to batching logic, misc tidy

* batch display notifications

* colocate all notification sending logic

* tidy tests

* remove unregister token for now

* fix registerPush lexicon, make a procedure

* fix registerPush impl, test pds proxy to notif service

* fix tests, make notif server optional when not configured

* fix notif server config for bsky app service

* move notif server rate limiting in-mem for now, add sending retry

* codegen tidy

* only push notifs on commit

* build

* fix notif rate limiter check

* send notifs from users w/o a profile

* remove build

---------

Co-authored-by: dholms <dtholmgren@gmail.com>
Co-authored-by: Devin Ivy <devinivy@gmail.com>
mloar pushed a commit to mloar/atproto that referenced this pull request Nov 15, 2023
* pseudocode for sending notification to user

* add notification push token table

* lexicon codegen

* `pds` and `api` codegen

* update lexicon

* add simple function to `putNotificationPushToken` into `notification_push_token` table

* reorgnaize imports

* add unspecced `putNotificationPushToken` to pds

* add `notification-push-tokens` table to PDS

* check if token exists before adding it to db

* add endpoint and appId to PDS table

* setup notification server

* fix logic for inserting token into db

* fix NotificationServer methods by making them static

* fix merge conflicts

* add comments on how sending ntoifications work

* remove dead code

* move notifServer to AppContext

* refactor code to handle notif sending in `Indexer` service

* add additional data when sending notifs

* clean up code

* move notif logic to indexer

* add `appId` and `endpoint` optional params to putNotificationPushToken

* clone notification code to AppView

* add endpoint to register push token with app view

* disable pds and enable app view notification server

* clean up code

* simply logic to check if a token already exists

* remove NotificationServer from PDS

* remove notification-push-token table from PDS

* remove `putNotificationPushToken` endpoint

* clean up code

* let `axios` throw error if `gorush` has an error

* let `kysely` throw error if notif cannot be registered by client

* rename `registerPushNotificationEndpoint` to `registerPushNotifications`

* delete `putNotificationPushToken` from AppView

* rename putPushNotificationToken to registerPushNotification

* remove dead notification code from pds

* remove sanitizeDisplayName from NotificationSever

* move `pushNotificationEndpoint` to config

* temp add `pushNotificationEndpoint` to dev-env setup

* remove example test from feed-generation.test.ts

* add test for registerPushNotification and clean up error handling

* move notifications test to its own file

* add test for NotificationServer to check if tokens are retrieved correctly

* remove unused functions from NotificationServer

* add additional tests for NotificationServer

* add return type to getNotificationDisplayAttributes function

* remove unnecessary comments

* remove dead NotificationServer code from PDS

* clean up code to prepareNotifsToSend

* put sending notifications as part of the backgroundQueue

* log instead of throwing error if notification attributes don't exist

* remove logs

* add more tests to `notification-server.test.ts`

* show replied with text for reply notifs

* better error handling when sending notifications via backgroundQueue

* add rate limit and batching to sending notifications

* add comments to NotificationServer

* merge with main

* use redis for rate limiting instead of normal rate limits

* move `notificationBatchSize` into config

* usePrimary db in test

* hoist push notif migration to present, update model to remove endpoint

* update push notif lexicon

* pare down lex for unregistering push

* helpers for working with service endpoints from did doc

* service-authed register/unregister push notifs

* add well-known endpoint to appview

* update bsky notif service tests

* fix to batching logic, misc tidy

* batch display notifications

* colocate all notification sending logic

* tidy tests

* remove unregister token for now

* fix registerPush lexicon, make a procedure

* fix registerPush impl, test pds proxy to notif service

* fix tests, make notif server optional when not configured

* fix notif server config for bsky app service

* move notif server rate limiting in-mem for now, add sending retry

* codegen tidy

* only push notifs on commit

* build

* fix notif rate limiter check

* send notifs from users w/o a profile

* remove build

---------

Co-authored-by: dholms <dtholmgren@gmail.com>
Co-authored-by: Devin Ivy <devinivy@gmail.com>
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.

4 participants