Skip to content

feat: default session id #122

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

Merged
merged 6 commits into from
Apr 23, 2025
Merged

feat: default session id #122

merged 6 commits into from
Apr 23, 2025

Conversation

kwasniew
Copy link
Contributor

@kwasniew kwasniew commented Apr 23, 2025

About the changes

Provide default session id if there's no session id from the user context. The session is is only consistent within the same process. To persist session across different process runs we'd need to put it into a storage. Our storage is only working with flags for now so it's a bigger change.
The reason why the in-process stable session is still valuable is the reduction of 200 requests and increase of 304s instead. With no fixed session any multi variant flag invalidates the cache since you may get assigned variant a or b on each request.

Closes #119

Important files

Discussion points

@@ -62,7 +62,7 @@ public class UnleashClientBase {
self.metrics = Metrics(appName: appName, metricsInterval: Double(metricsInterval), clock: { return Date() }, disableMetrics: disableMetrics, poster: urlSessionPoster, url: url, clientKey: clientKey, customHeaders: customHeaders, connectionId: connectionId)
}

self.context = Context(appName: appName, environment: environment)
self.context = Context(appName: appName, environment: environment, sessionId: String(Int.random(in: 0..<1_000_000_000)))

Choose a reason for hiding this comment

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

QQ is calculateContext overriding the sessionId with the providedContext session id (if present)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link

@gastonfournier gastonfournier left a comment

Choose a reason for hiding this comment

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

LGTM!

@github-project-automation github-project-automation bot moved this from New to Approved PRs in Issues and PRs Apr 23, 2025
@@ -192,12 +192,14 @@ public class UnleashClientBase {
properties?.forEach { (key, value) in
newProperties[key] = value
}

let sessionId = context["sessionId"] ?? self.context.sessionId;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gastonfournier here's the override if present

@kwasniew kwasniew merged commit c747f79 into main Apr 23, 2025
4 checks passed
@kwasniew kwasniew deleted the default-session-id branch April 23, 2025 11:15
@github-project-automation github-project-automation bot moved this from Approved PRs to Done in Issues and PRs Apr 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Provide a default SessionId value for Unleash context
2 participants