-
Notifications
You must be signed in to change notification settings - Fork 25
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
Conversation
@@ -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))) |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -192,12 +192,14 @@ public class UnleashClientBase { | |||
properties?.forEach { (key, value) in | |||
newProperties[key] = value | |||
} | |||
|
|||
let sessionId = context["sessionId"] ?? self.context.sessionId; |
There was a problem hiding this comment.
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
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