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 #271

Merged
merged 4 commits into from
Jan 29, 2025
Merged

feat: client identification headers #271

merged 4 commits into from
Jan 29, 2025

Conversation

Tymek
Copy link
Member

@Tymek Tymek commented Jan 28, 2025

Description

Identification headers consistent with other Unleash SDKs.

This PR adds standardized 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 will be deleted in another PR (expand/contract pattern)
  • x-unleash-connection-id: an internal 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-client-<language>:<version>

All the headers are intended for the Unleash team. Changes should be implemented in a way that does not change SDK behavior in a significant way.
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

Fixes issue/1-3275

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Running unit tests locally
  • Starting example app and verifying headers sent over the network

Test Configuration:

  • Linux, dotnet 6.0.135

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@Tymek Tymek marked this pull request as ready for review January 29, 2025 09:53
@Tymek Tymek requested a review from kwasniew January 29, 2025 09:53
Copy link
Contributor

@kwasniew kwasniew left a comment

Choose a reason for hiding this comment

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

I find it weird that we can overwrite unleash-client-mock. I'd expect that it's always unleash-client-dotnet. Also how do we read the real version here?

@@ -84,6 +85,8 @@ public UnleashServices(UnleashSettings settings, EventCallbackConfig eventConfig
{
AppName = settings.AppName,
InstanceTag = settings.InstanceTag,
ConnectionId = connectionId,
SdkVersion = settings.SdkVersion,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kwasniew don't know if this answers your questions, but the "real" header value is read here from a default implementation in UnleashSettings: https://github.com/Unleash/unleash-client-dotnet/blob/main/src/Unleash/UnleashSettings.cs#L144C31-L144C44

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd fix the original versioning to follow unleash-client-dotnet:semver convention and drop the v prefix

Copy link
Contributor

Choose a reason for hiding this comment

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

one drawback is that there's a lot of existing dotnet SDKs using the v version scheme

Copy link
Collaborator

@daveleek daveleek left a comment

Choose a reason for hiding this comment

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

LGTM!

@Tymek Tymek merged commit e66372c into main Jan 29, 2025
8 checks passed
@Tymek Tymek deleted the 1-3275 branch January 29, 2025 11:29
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