Skip to content

feat: client identification headers #342

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jan 29, 2025
Merged

feat: client identification headers #342

merged 5 commits into from
Jan 29, 2025
+76 −20

Conversation

Tymek
Copy link
Member

@Tymek Tymek commented Jan 27, 2025 •
edited
Loading

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-3274

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.

  • Unit tests
  • Spec Tests
  • Integration tests / Manual Tests

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
PHP-Proxy

PHP-Proxy

Error accessing resource: 404 - Not Found

@Tymek Tymek changed the title test: identification headers presence Identification headers Jan 27, 2025
@Tymek Tymek changed the title Identification headers SDK Identification headers Jan 27, 2025
@Tymek Tymek changed the title SDK Identification headers SDK identification headers Jan 27, 2025
@Tymek Tymek requested a review from kwasniew January 27, 2025 13:27
@Tymek Tymek marked this pull request as ready for review January 27, 2025 13:28
@coveralls
Copy link

coveralls commented Jan 27, 2025 •
edited
Loading

Coverage Status

coverage: 95.327% (+0.01%) from 95.316%
when pulling 2f4c889 on 1-3274
into b46cd17 on main.

@Tymek Tymek changed the title SDK identification headers feat: client identification headers Jan 27, 2025
kwasniew
kwasniew reviewed Jan 27, 2025
View reviewed changes
sighphyre
sighphyre reviewed Jan 27, 2025
View reviewed changes
UnleashClient/__init__.py Outdated
# Setup
metrics_args = {
"url": self.unleash_url,
"app_name": self.unleash_app_name,
"instance_id": self.unleash_instance_id,
"custom_headers": self.unleash_custom_headers,
"custom_headers": headers,
Copy link
Member

Choose a reason for hiding this comment

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

I think it's okay to pass this like this but this does now mean that this isn't just custom headers and the places where the callees are receiving this are now misleading

Any chance we can rename those call sites to "headers" or something clearer?

Copy link
Member Author

Choose a reason for hiding this comment

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

Like so: fc4685f ?

@Tymek Tymek requested review from sighphyre and kwasniew January 27, 2025 16:33
@kwasniew
Copy link
Contributor

Can you run the example app https://github.com/Unleash/unleash-sdk-examples/tree/main/Python with the latest version after publishing this package and checking the headers work in the user space code?

@Tymek
Copy link
Member Author

Tymek commented Jan 29, 2025

Verified with real-world app

@Tymek Tymek merged commit 6799788 into main Jan 29, 2025
43 checks passed
@Tymek Tymek deleted the 1-3274 branch January 29, 2025 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kwasniew kwasniew

@sighphyre sighphyre

Assignees
No one assigned
Labels
None yet
Projects
Issues and PRs
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Tymek @coveralls @kwasniew @sighphyre