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

Update data source for getSuggestedFollowsByActor #2630

Merged
merged 20 commits into from
Jul 11, 2024
Merged

Conversation

estrattonbailey
Copy link
Member

@estrattonbailey estrattonbailey commented Jul 8, 2024

We've added a new parameter to our otherwise generic app.bsky.unspecced.getSuggestionsSkeleton called relativeToDid, which requests suggestions relative to that DID instead of that of the viewer. The intent here is to unify the algorithm we use to suggest follows to the viewer by replacing the algo that powers getSuggestedFollowsByActor with the one we use for our other suggestions in the app.

I've integrated this unspecced endpoint and new param into the existing endpoint and put it behind a feature flag so we can selectively turn it on to test.

To do so, this PR also integrates StatSig. I've added a mgmt class FeatureGates that lives on AppContext and integrates with the start and destroy lifecycle of the app's context.

It can be used within route handlers like this:

const gates = ctx.featureGates

if (gates.check(
  await gates.user({ did: '...' }),
  gates.ids.MyGateId
)) {
  // gate enabled, do something
}

In testing, StatSig is put into "local mode", which allows you to override flags for testing. See here for more info.

@estrattonbailey estrattonbailey marked this pull request as ready for review July 8, 2024 23:13
packages/bsky/src/index.ts Outdated Show resolved Hide resolved
packages/crypto/src/sha.ts Outdated Show resolved Hide resolved
packages/crypto/src/sha.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@devinivy devinivy left a comment

Choose a reason for hiding this comment

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

Lookin good, just left a few notes! The main ones are a. lifecycle of statsig, b. failing open when statsig is failing or not configured at all. I think they both could be addressed and brought into a common pattern used in the node services by making a new class for managing feature gates, which will live on the AppContext and used as part of the AppContext's lifecycle. Maybe could look roughly like this:

interface FeatureGates {
  private statsig: StatsigInstance | undefined
  constructor(config: StatsigConfig | undefined)
  // when statsig is being used, initializes statsig. called in AppContext's start().
  start(): Promise<void>
  // when statsig is being used, shuts-down statsig. called in AppContext's destroy().
  destroy(): Promise<void>
  // when statsig is being used returns statsig's gate, otherwise fails open with a log and returning the default.
  // when statsig is not being used returns a default for the gate (probably false).
  check(gate: Gate, userId: string): boolean
  getUserId(did: string): Promise<string>
}

Copy link
Collaborator

@devinivy devinivy left a comment

Choose a reason for hiding this comment

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

Clean!! Just a few tiiiny tweaks and I think we'll be rockin.

packages/bsky/src/index.ts Outdated Show resolved Hide resolved
@@ -161,10 +168,12 @@ export class BskyAppView {
await events.once(server, 'listening')
const { port } = server.address() as AddressInfo
this.ctx.cfg.assignPort(port)
await this.ctx.featureGates.start()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we'll want to move this up to before the server is listening / fielding requests.

Comment on lines 44 to 45
} catch (_) {
this.ready = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should at minimum log the error so we can see there's an issue—you'll find some loggers in ./logger and we can add a new one if we want!

},
})
this.ready = true
} else if (process.env.NODE_ENV === 'test') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be possible to hoist this behavior up either to config.ts or even the appview's statsig config in the tests? This isn't a major deal / can totally punt for now and just leave a note if it's a pain! The reason we've been trying to do things that way is just to avoid behaviors that don't flow from the app entrypoint/config, and perhaps a sprinkle of brain being a little wired to certain 12 factor thinking 🤣 .

Copy link
Collaborator

@devinivy devinivy left a comment

Choose a reason for hiding this comment

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

Ace!

@devinivy devinivy merged commit 8f22a25 into main Jul 11, 2024
10 checks passed
@devinivy devinivy deleted the get-suggestions-update branch July 11, 2024 21:14
estrattonbailey added a commit that referenced this pull request Jul 16, 2024
…cements

* origin/main: (181 commits)
  Minor OAuth client fixes (#2640)
  Version packages (#2639)
  OAuth: 2FA (#2633)
  Version packages (#2622)
  Update data source for `getSuggestedFollowsByActor` (#2630)
  Add in-memory did cache to Ozone backend (#2635)
  Filter out reference lists from `getLists` (#2629)
  lexicons: add missing ozone Tag event type to unions (#2632)
  ✨ Add ozone proxy for getLikes and getRepostedBy (#2624)
  Upgrade pnpm/action-setup in workflows (#2625)
  ✨ Add proxy for user typeahead through ozone (#2612)
  Fix development commands (#2623)
  Add starter packs to post hydration (#2613)
  Social proof blocks (#2603)
  Appview: apply hosting status in getRecord (#2620)
  Add `labelersPref` to `getPreferences` union return types (#2554)
  Version packages (#2618)
  Add new preference and api for bsky app state; also put preference updates within transactional lock regions (#2492)
  remove mentions of sandbox (#2611)
  Appview: simple fix for no-hosted known followers (#2609)
  ...
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.

2 participants