Skip to content

chore(flags): use new /flags endpoint instead of /decide #345

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

dmarticus
Copy link
Contributor

@dmarticus dmarticus commented Apr 28, 2025

💡 Motivation and Context

Update the iOS SDK to use the new /flags endpoint instead of the legacy /decide one. Part of the /flags global rollout PostHog/posthog#33247

This should be a safe change because /flags&config=true returns the exact same fields as /decide does, so we don't need to worry about missing any non-feature flag related fields. This is in response to this comment: #345 (comment)

@dmarticus dmarticus requested a review from marandaneto as a code owner April 28, 2025 23:27
@marandaneto marandaneto requested a review from ioannisj April 29, 2025 05:56
@marandaneto
Copy link
Member

@ioannisj can you review it?
Also, I'd debug #344 first since its a different endpoint now (and likely easier to reproduce if there's a bug)

@ioannisj
Copy link
Contributor

@dmarticus Getting a 400 Bad Request - Failed to decode request: unsupported content type: application/json; charset=utf-8. Please check your request format and try again. when testing on a demo project.

Other than this and reverting the commit from ./scripts/prepare-release.sh that @marandaneto mentioned, LG to me. I don't see any other logic other than changing the endpoint affected

@dmarticus
Copy link
Contributor Author

@ioannisj I fixed the issue with PostHog/posthog#31798, can you give it a try on your side again?

@dmarticus dmarticus requested a review from marandaneto May 1, 2025 17:32
@ioannisj
Copy link
Contributor

ioannisj commented May 2, 2025

@ioannisj I fixed the issue with PostHog/posthog#31798, can you give it a try on your side again?

Yeah getting a response now.
We need to think how sessionRecording should be handled though cause we depend on that on /decide endpoint (talked about this privately already)

@marandaneto
Copy link
Member

similar to PostHog/posthog-android#245 (comment) but here https://github.com/PostHog/posthog-ios/blob/479ce5ba9ce899dfe969dea7c88cd110adb800e7/PostHog/PostHogRemoteConfig.swift#L187C18-L187C35 and probably more places, @ioannisj would know more

@dmarticus
Copy link
Contributor Author

@ioannisj with this commit ce2f2ac (#345) I've added support for all of the /decide fields to the /flags endpoint, how does this look to you?

) else {
hedgeLog("Malformed decide URL error.")
let url: URL?
if config.remoteConfig {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't need to load the rest of the config if we're using RemoteConfig

Copy link
Contributor

@ioannisj ioannisj Jun 11, 2025

Choose a reason for hiding this comment

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

Hey @dmarticus, I think this won't fix the session replays that depend on flags issue. We do fetch sessionRecording from remote config to get linkedFlag value, but the way the code is currently structured the check expects both sessionRecording and feature flag values to be present when calling isRecordingActive

As it stands, session recording will only be evaluated from remote config payload (if config.remoteConfig is true), but it will always do so with cached flags (from a previous app run).

  1. enable session recording flag on dashboard
  2. first run, session recording evaluates to false (no cached flags)
  3. second run, session recording evaluates to true (from previously cached flags)
  4. turn off session recording flag on dashboard
  5. app run, session recording evaluates to true (from previously cached flags)
  6. additional run, session recording correctly evaluates to false

So here we can either:

  1. restructure the code so that we cache linkedFlag from remote config call and re-evaluate when /flags is called with fresh flag values
  2. always call with /flags&config=true so that we get the whole response for now

cc: @marandaneto can fact check me on this

Comment on lines 208 to 214
} else {
url = getEndpointURL(
"/flags",
queryItems: URLQueryItem(name: "v", value: "2"), URLQueryItem(name: "config", value: "true"),
relativeTo: config.host
)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

btw i fully used Claude on this, not sure if it's idiomatic.

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.

3 participants