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.

Sign up for GitHub

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(feature-flags): support quota limiting for feature flags #228

Merged
merged 9 commits into from
Feb 25, 2025
Merged

feat(feature-flags): support quota limiting for feature flags #228

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4e40734
added a test too
dmarticus Feb 22, 2025
3673ff5
formatting
dmarticus Feb 22, 2025
61358a6
more formatting and also simpler test data
dmarticus Feb 22, 2025
21991d0
update changelog
dmarticus Feb 22, 2025
9813f99
code review
dmarticus Feb 23, 2025
2b66513
fix recursion
marandaneto Feb 24, 2025
62fec9f
fix
marandaneto Feb 24, 2025
1493a40
fix lint
marandaneto Feb 24, 2025
d15a895
Update CHANGELOG.md
dmarticus Feb 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
## Next

## 3.11.3 - 2025-02-21

- feat: support quota limiting for feature flags ([#228](https://github.com/PostHog/posthog-android/pull/228))

## 3.11.2 - 2025-02-04

- fix: touches fall back to single touches if out of bounds ([#221](https://github.com/PostHog/posthog-android/pull/221))
Expand Down
2 changes: 2 additions & 0 deletions posthog/src/main/java/com/posthog/internal/PostHogDecideResponse.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import org.codehaus.mojo.animal_sniffer.IgnoreJRERequirement
* @property errorsWhileComputingFlags if there were errors computing feature flags
* @property featureFlags the feature flags
* @property featureFlagPayloads the feature flag payloads
* @property quotaLimited array of quota limited features
*/
@IgnoreJRERequirement
internal data class PostHogDecideResponse(
Expand All @@ -16,4 +17,5 @@ internal data class PostHogDecideResponse(
val featureFlagPayloads: Map<String, Any?>?,
// its either a boolean or a map, see https://github.com/PostHog/posthog-js/blob/10fd7f4fa083f997d31a4a4c7be7d311d0a95e74/src/types.ts#L235-L243
val sessionRecording: Any? = false,
val quotaLimited: Array<String>? = null,
)
11 changes: 11 additions & 0 deletions posthog/src/main/java/com/posthog/internal/PostHogFeatureFlags.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,17 @@ internal class PostHogFeatureFlags(

response?.let {
synchronized(featureFlagsLock) {
if (response.quotaLimited?.contains("feature_flags") == true) {
config.logger.log("Feature flags are quota limited, clearing existing flags")
this.featureFlags = null
this.featureFlagPayloads = null
config.cachePreferences?.let { preferences ->
preferences.remove(FEATURE_FLAGS)
preferences.remove(FEATURE_FLAGS_PAYLOAD)
}
Comment on lines +96 to +99
Copy link
Member

Choose a reason for hiding this comment

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

what should happen if session replay uses a feature flag and now is quota limited, should it stop working as well? if so, I guess we can also do preferences.remove(SESSION_REPLAY) if session replay has a linkedFlag value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yeah, I didn't think of that scenario – can you tell me more about how session replay with flags works? I'm fine removing it but I don't really understand how session replay and flags overlap in this context.

Copy link
Member

Choose a reason for hiding this comment

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

@Posthog/team-replay would know more but the implementation is straightforward.
https://github.com/PostHog/posthog-android/blob/41ac8e9dbf989348823052c8c0f14c9c0d1fa2fd/posthog/src/main/java/com/posthog/internal/PostHogFeatureFlags.kt#L39C17-L68
https://github.com/PostHog/posthog/blob/31427d51f6a4aa91b45b3f4c49ef019f22c503d3/posthog/api/decide.py#L489
if /decide returns a linkedFlag value, that means we have to evaluate this flag locally, so the current approach will break session replay even if you have recordings quota.
now the question is, if you have recordings quota, you don't have flags quota, but session replay configs is enabled behind a flag, should it work or not?

Copy link
Member

Choose a reason for hiding this comment

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

well, it should fail closed

generally speaking folk are setting a linked flag to reduce cost/volume. so failing open would be nasty

so, I have a linked flag, but I am past flag quota, decide/config continues to return the linked flag for replay, the SDK never shows it as received because i'm past quota, and replay never starts

Copy link
Member

Choose a reason for hiding this comment

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

then i think the UX is all around how we notify people about being past quota. feels way more important for flags than other products

Copy link
Member

Choose a reason for hiding this comment

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

the current approach will silently disable session replay since the linked flag won't exist, which will be one more edge case to debug missing recordings on SDKs.
is that the expectation?

ps: there's the quota limiting logging but it's not really specific about session replay, so folks won't get it right away, I think.

Copy link
Member

Choose a reason for hiding this comment

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

yep, at least failing closed :)

ideally we'd register a super property on events if replay or flags or etc are past their quota for debugging (obvs doesn't help if events are past quota)

but yep, the onward journey will be the thing

return@executeSafely
}

if (response.errorsWhileComputingFlags) {
// if not all flags were computed, we upsert flags instead of replacing them
this.featureFlags =
Expand Down
45 changes: 45 additions & 0 deletions posthog/src/test/java/com/posthog/internal/PostHogFeatureFlagsTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,4 +434,49 @@ internal class PostHogFeatureFlagsTest {

sut.clear()
}

@Test
fun `clear flags when quota limited`() {
val http =
mockHttp(
response =
MockResponse().setBody(
"""
{
"featureFlags": {"flag1": true},
"featureFlagPayloads": {"flag1": "payload1"}
}
""".trimIndent(),
),
)
val url = http.url("/")
val sut = getSut(host = url.toString())

// Load initial flags
sut.loadFeatureFlags("test_id", null, null, null)
executor.awaitExecution()

// Verify flags are loaded
assertNotNull(sut.getFeatureFlags())
assertNotNull(preferences.getValue(FEATURE_FLAGS))

// Send quota limited response
http.enqueue(
MockResponse().setBody(
"""
{
"quotaLimited": ["feature_flags"]
}
""".trimIndent(),
),
)

// Reload flags
sut.loadFeatureFlags("test_id", null, null, null)
executor.awaitExecution()

// Verify flags are cleared
assertNull(sut.getFeatureFlags())
assertNull(preferences.getValue(FEATURE_FLAGS))
}
}
Loading