feat: client identification headers #89

kwasniew · 2025-01-28T13:40:33Z

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 lib
x-unleash-sdk: sdk information in the format unleash-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:

capacity planning by knowing the number of unique connections made to the backend API
debugging misconfigured SDKs sending more traffic than expected

Important files

Discussion points

I added 2 new dependencies deliberately choosing versions that work on 1.60.0. However some transitive dependencies of existing dependencies have changed and require 1.60.3. I found that any dependency update will trigger this error. Maybe we can update MSRV?

Also we have a new way of adding library version at compile time from the Cargo file.

Cargo.toml

kwasniew · 2025-01-28T13:43:33Z

src/http.rs

@@ -36,10 +43,15 @@ where
        Ok(HTTP {
            client: C::default(),
            app_name,
+            sdk_version: get_sdk_version(),
+            connection_id: Uuid::new_v4().into(),
            instance_id,
            authorization,
            authorization_header: C::build_header("authorization")?,
            app_name_header: C::build_header("appname")?,


this header will be deprecated in the next version since we're moving towards standardized x-unleash headers

I haven't seen these headers before. Are they already in production somewhere?

If not, please see https://datatracker.ietf.org/doc/html/rfc6648 - x- is a deprecated pattern and we SHOULD NOT introduce it now.

We started adding them 2 weeks ago for our internal purposes. Those are not useful to Unleash users and only used internally so we have more freedom in changing them in future. You raise a good point with the x-prefix. I'll discuss it with the team tomorrow (if it's worth changing it given all other SDKs already support it). We may have some legacy users who require the x- prefix in their network infra.

Update: after internal discussions we decided to drop the x- prefix and we'll stick to unleash-. The spec link you sent is convincing :)

kwasniew · 2025-01-28T13:44:56Z

src/api.rs

@@ -101,7 +102,7 @@ impl Default for Registration {
        Self {
            app_name: "".into(),
            instance_id: "".into(),
-            sdk_version: "unleash-client-rust-0.1.0".into(),


this version was out of sync with the cargo version

also in out backend we assume a different format unleash-client-rust:version
I decided to create a function capturing those requirements

kwasniew · 2025-01-28T13:45:42Z

src/version.rs

@@ -0,0 +1,23 @@
+use std::env;
+
+// include version into the binary at compile time


It is very important to read the version env var at compile time so that it's hardcoded into the final binary for predictability and reading the library version and not the version of the application using this library

kwasniew · 2025-01-28T13:48:19Z

src/http.rs

@@ -36,10 +43,15 @@ where
        Ok(HTTP {
            client: C::default(),
            app_name,
+            sdk_version: get_sdk_version(),
+            connection_id: Uuid::new_v4().into(),


I decided to instantiate the UUID when we create our HTTP client since we don't need it anywhere else. Since this is intended for internal tracking purposes of SDK HTTP connection and users should not override it I put it here instead of passing it from the outside.

Does this make sense for the Rust SDK as-is? A new UUID every process restart doesn't provide much value. (And indeed for k8s we'd see 10's of thousands of clients a week...

We've been using it for 2 weeks already and it provides a lot of value to us on the server side. We want to know how many instances are created. We track it in hourly buckets with hyperloglog so very high numbers are not a problem and they help us in capacity planning and debugging of misconfigured SDK clients (with per request scope objects)

src/version.rs

kwasniew · 2025-01-28T14:05:26Z

Cargo.toml

@@ -62,6 +62,7 @@ rustversion = "1.0.7"
 serde_json = "1.0.68"
 serde_plain = "1.0.0"
 surf = { version = "2.3.1", optional = true }
+uuid = { version = "0.8.2", features = ["v4"] }


highest version running on rustc 1.60.0. I also considered embedding uuid generation function into the library itself but my first option is to use a lib.

kwasniew · 2025-01-28T15:15:52Z

Cargo.toml

@@ -62,6 +62,7 @@ rustversion = "1.0.7"
 serde_json = "1.0.68"
 serde_plain = "1.0.0"
 surf = { version = "2.3.1", optional = true }
+uuid = { version = "1.11.0", features = ["v4"] }


latest 1.60.0 supported version

kwasniew · 2025-01-28T15:15:55Z

Cargo.toml

@@ -91,6 +92,7 @@ futures = "0.3.17"
 maplit = "1.0.2"
 num_cpus = "1.13.0"
 simple_logger = "2.1.0"
+regex = "1.9.6"


latest 1.60.0 supported version

chriswk

I'm a 👍 here.

chriswk · 2025-01-29T08:44:39Z

I'm a 👍 here.

but we will have to hash out the MSRV details. I know @sighphyre is already looking

rbtcollins

I will read the code more shortly, but a quick note:

x- headers are deprecated.

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 lib
x-unleash-sdk: sdk information in the format unleash-client-android:

should instead be something without x-.

src/version.rs

rbtcollins · 2025-01-29T16:51:27Z

src/http.rs

    instance_id: String,
+    connection_id: String,


This is being used as a client id, not per-connection

Suggested change

connection_id: String,

client_id: String,

We have an established convention across all SDKs so I'd prefer to have one name for this. We call it connection_id because we're also adding streaming support and we want to treat a long lived SSE stream and interval/polling connections from the same instance as one logical connection.

I agree that consistency matters, however it is still mismatched with the code behaviour.

Personally, I would name it differently to capture these nuances in the other clients, but thats out of my hands :)

I will ask for a comment though, explaining why this is considered a 'connection id', even though its not per HTTP connection, and not unique per process.

rbtcollins · 2025-01-29T16:53:47Z

This has no test coverage; someone could change it later and we'd have nothing capturing what or why they shouldn't.

kwasniew · 2025-01-29T20:34:38Z

@rbtcollins Thanks for a thorough review.
I added tests checking the new headers and existing ones.
I'm also interested in your take on the MSRV bump.

feat: client identification headers

d07211b

kwasniew commented Jan 28, 2025

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

kwasniew commented Jan 28, 2025

View reviewed changes

src/version.rs Outdated Show resolved Hide resolved

kwasniew added 3 commits January 28, 2025 14:52

feat: client identification headers

4d58cf7

feat: client identification headers

ebad2ad

feat: client identification headers

d38b17e

kwasniew commented Jan 28, 2025

View reviewed changes

kwasniew added 4 commits January 28, 2025 15:13

feat: client identification headers

44367bc

feat: client identification headers

044fb89

feat: client identification headers

f64c42f

feat: client identification headers

9f1099e

kwasniew commented Jan 28, 2025

View reviewed changes

kwasniew requested review from sighphyre, chriswk, thomasheartman, rbtcollins and Tymek January 28, 2025 15:16

chriswk approved these changes Jan 29, 2025

View reviewed changes

rbtcollins requested changes Jan 29, 2025

View reviewed changes

rbtcollins reviewed Jan 29, 2025

View reviewed changes

src/version.rs Outdated Show resolved Hide resolved

rbtcollins reviewed Jan 29, 2025

View reviewed changes

src/version.rs Outdated Show resolved Hide resolved

rbtcollins reviewed Jan 29, 2025

View reviewed changes

src/version.rs Show resolved Hide resolved

rbtcollins reviewed Jan 29, 2025

View reviewed changes

kwasniew added 8 commits January 30, 2025 09:46

chore: make module pub crate

26c8e50

docs: version module

b0eaa6e

test: http headers

34efcba

test: http headers

d59c9f5

feat: use custom headers without x

8c707bd

feat: use custom headers without x

b922bd5

fix: clippy

581dee7

fix: clippy

ee88211

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: client identification headers #89

feat: client identification headers #89

kwasniew commented Jan 28, 2025 •

edited

Loading

kwasniew Jan 28, 2025

rbtcollins Jan 29, 2025

rbtcollins Jan 29, 2025

kwasniew Jan 29, 2025 •

edited

Loading

kwasniew Jan 30, 2025

kwasniew Jan 28, 2025

kwasniew Jan 28, 2025

kwasniew Jan 28, 2025

rbtcollins Jan 29, 2025

kwasniew Jan 29, 2025

kwasniew Jan 28, 2025

kwasniew Jan 28, 2025

kwasniew Jan 28, 2025

chriswk left a comment

chriswk commented Jan 29, 2025

rbtcollins left a comment

rbtcollins Jan 29, 2025

kwasniew Jan 29, 2025

rbtcollins Jan 30, 2025

rbtcollins commented Jan 29, 2025

kwasniew commented Jan 29, 2025 •

edited

Loading

		@@ -0,0 +1,23 @@
		use std::env;

		// include version into the binary at compile time

feat: client identification headers #89

Are you sure you want to change the base?

feat: client identification headers #89

Conversation

kwasniew commented Jan 28, 2025 • edited Loading

About the changes

Important files

Discussion points

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwasniew Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriswk left a comment

Choose a reason for hiding this comment

chriswk commented Jan 29, 2025

rbtcollins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbtcollins commented Jan 29, 2025

kwasniew commented Jan 29, 2025 • edited Loading

kwasniew commented Jan 28, 2025 •

edited

Loading

kwasniew Jan 29, 2025 •

edited

Loading

kwasniew commented Jan 29, 2025 •

edited

Loading