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.

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: client identification headers #690

Merged
merged 2 commits into from
Jan 8, 2025
Merged

Conversation

kwasniew
Copy link
Contributor

@kwasniew kwasniew commented Jan 7, 2025

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. UNLEASH-APPNAME and User-Agent will be deleted in another PR
  • x-unleash-connection-id: a unique identifier for the current instance of the client generated by the built-in crypto lib
  • x-unleash-sdk: sdk information in the format unleash-node@<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

Interesting parts:

  • reading version from details.json using require
  • testing header factory, repository usage of headers, metrics client usage of headers and main file generating initial uuid

Important files

Discussion points

nock(url).get('/client/features').reply(200, { features: toggles });
nock(url)
.get('/client/features')
.matchHeader('x-unleash-connection-id', /^.{36}$/)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

every request to unleash should have 36 digit uuid

@coveralls
Copy link

coveralls commented Jan 7, 2025

Coverage Status

coverage: 90.53% (+0.07%) from 90.462%
when pulling 5c7ae36 on client-identification-headers
into dcbb847 on main.

thomasheartman
thomasheartman previously approved these changes Jan 8, 2025
Copy link
Contributor

@thomasheartman thomasheartman left a comment

Choose a reason for hiding this comment

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

Nice one!

Comment on lines +82 to +84
// TODO: delete
head['UNLEASH-APPNAME'] = appName;
// TODO: delete
Copy link
Contributor

Choose a reason for hiding this comment

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

delete when, though? Next major?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either next major or at least another PR to avoid expanding and contracting headers in the same one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to have a super safe version upgrade that I can recommend to everyone even if for some unknown reason they relied on he old headers

@@ -177,12 +183,15 @@ test('should request with etag', (t) =>
repo.start();
}));

test('should request with custom headers', (t) =>
test('should request with correct headers', (t) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I see why you're co-opting the tests here, but we're also losing a bit of information about the tests with this rename. Before, it was clear that we're talking about custom headers, so if you're scanning through the tests, it was easy enough to find (or grep for). "Correct headers" is more ... ambiguous. How about renaming this one to something like this?

Suggested change
test('should request with correct headers', (t) =>
test('should request with custom and `x-unleash` headers', (t) =>

The same goes for the other tests that were changed from "custom" to "correct".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I also updated one more test with the same change

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.

3 participants