-
Notifications
You must be signed in to change notification settings - Fork 4
feat: client identification headers #103
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
| return strategy.httpCustomHeaders.plus(mapOf( | ||
| "Authorization" to auth, | ||
| "Content-Type" to "application/json", | ||
| "UNLEASH-APPNAME" to appName, |
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.
will be deprecated after expand/contract period
| "User-Agent" to appName, | ||
| "UNLEASH-INSTANCEID" to instanceId, | ||
| "X-UNLEASH-CONNECTION-ID" to instanceId, | ||
| "X-UNLEASH-SDK" to "unleash-client-android:development", |
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.
we need to find a way to get current version instead of development
| compileSdk = 34 | ||
|
|
||
| buildFeatures { | ||
| buildConfig = true |
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.
enable build config that auto generated BuildConfig file
| testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner" | ||
| consumerProguardFiles("proguard-rules.pro") | ||
|
|
||
| buildConfigField("String", "VERSION", "\"${project.version}\"") |
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.
add custom VERSION field to the BuildConfig
gastonfournier
left a comment
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.
it seems that instanceId is not exposed to users in this SDK so I can re-use it for connection id
This was an explicit decision during the design as instanceId should identify the instance and not be overridden. Maybe we should pursue this in all our SDKs.
The PR looks good!
| "X-UNLEASH-APPNAME" to appName, | ||
| "User-Agent" to appName, | ||
| "UNLEASH-INSTANCEID" to instanceId, | ||
| "X-UNLEASH-CONNECTION-ID" to instanceId, |
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.
I hope we can deprecate UNLEASH-INSTANCEID header, as this connection id seems to be fulfilling the same purpose. We should open that conversation, but definitely outside of this PR scope
We did try, turns out that breaks Gitlab, which is why we rolled that back. We should still move in that direction though, agreed |
About the changes
This PR adds standardised client identification headers to the feature and metrics calls that the client makes to Unleash. The headers are:
x-unleash-appname: the name of the application that is using the client.x-unleash-connection-id: a unique identifier for the current instance of the client generated by the built-in UUID libx-unleash-sdk: sdk information in the formatunleash-client-android:<version>All the headers are intended for the Unleash team so clients should not be affected.
The main use cases we have are:
Important files
Discussion points
In development the generated BuildConfig looks like this:
The VERSION has a branch name and snapshot. I hope that after proper release it will get updated to a regular version.