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.

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

[RED-2503][RED-2506][RED-2507][RED-2509] Add zaf_sdk user agent to headers for requests #220

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

[RED-2503][RED-2506][RED-2507][RED-2509] Add zaf_sdk user agent to headers for requests #220

wants to merge 6 commits into from

Conversation

token-cjg
Copy link
Contributor

@token-cjg token-cjg commented Nov 4, 2024 •
edited
Loading

✌️

/cc @zendesk/vegemite

Description

  • To assist with debugging the source of various requests it would be helpful to indicate whether requests have been touched by the zaf_sdk at any point
  • This will help operationally for various things that need to be done to monitor things of an API related nature
  • Knowing that a request has been touched by zaf_sdk helps to pinpoint the source + narrow down where things need fixing and/or patching

Tasks

  • Include comments/inline docs where appropriate
  • Add unit tests
  • Update changelog here
  • Add label from one of 'patch'/'minor'/'major' if you want to create a release when merged

References

DevQA Steps

NOTE: DevQA steps are to be actioned only once code has been reviewed and approved.

Risks

  • Medium: Touches requests made by the zaf_sdk, but should not incur too much risk as I'm touching the user-agent header which shouldn't be on the critical path.

Rollback Plan

  1. Quickly roll back to the prior release so that customers can refresh to resolve their issue.
  2. Revert this PR to restore the master branch to a deployable green state.
  3. Notify the author.

@token-cjg
Indicate how to install node 12 on Apple Silicon
ca0d4ac
@token-cjg
Add zaf_sdk user agent to headers for requests
3a9251d 
- To assist with debugging the source of various requests it would be helpful
  to indicate whether requests have been touched by the zaf_sdk at any point
- This will help operationally for various things that need to be done to
  monitor things of an api related nature
- Knowing that a request has been touched by zaf_sdk helps to pinpoint the
  source + narrow down where things need fixing and/or patching
- It would be good to get the appId too but this is a good first port of call
@token-cjg token-cjg force-pushed the cgoddard/red-2506/add-user-agent branch from 3bb3748 to 3a9251d Compare November 4, 2024 06:18
@token-cjg token-cjg marked this pull request as ready for review November 5, 2024 21:32
@token-cjg
Consume app_id on app registration + pass /w user-agent on request
091e70b 
- Let's ensure that we know which app made a given request by encoding within
  the user-agent
- This will help /w ascertaining who is making a given request
@token-cjg token-cjg force-pushed the cgoddard/red-2506/add-user-agent branch from 1e24b30 to 091e70b Compare November 5, 2024 21:38
@token-cjg token-cjg changed the title [RED-2506] Add zaf_sdk user agent to headers for requests [RED-2506][RED-2507] Add zaf_sdk user agent to headers for requests Nov 5, 2024
@token-cjg token-cjg changed the title [RED-2506][RED-2507] Add zaf_sdk user agent to headers for requests [RED-2503][RED-2506][RED-2507] Add zaf_sdk user agent to headers for requests Nov 5, 2024
caspersg
caspersg reviewed Nov 6, 2024
View reviewed changes
lib/utils.js Outdated
const Promise = window.Promise || NativePromise

export function addUserAgent (headers, appId) {
Copy link

Choose a reason for hiding this comment

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

You could add a test for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actioned with this commit 23e6df8 , thanks for keeping my pull requests honest 😅

@caspersg
Copy link

caspersg commented Nov 6, 2024

FYI: looks like you need to complete the Tasks too.

Shengming-Yan
Shengming-Yan reviewed Nov 6, 2024
View reviewed changes
lib/utils.js Outdated Show resolved Hide resolved
Shengming-Yan
Shengming-Yan reviewed Nov 6, 2024
View reviewed changes
lib/utils.js
const Promise = window.Promise || NativePromise

export function addUserAgent (headers, appId) {
const originalUserAgent = headers && headers['User-Agent'] ? headers['User-Agent'] : ''
const zafSdkUserAgentString = `zendesk_app_framework_sdk/sdk_version:${pkgJson.version}/app_id:${appId}`
Copy link
Contributor

Choose a reason for hiding this comment

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

I reckon the version in pkgJson doesn't reflect the actual version, this PR didn't touch on the version as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you Sheen, should I remove this? Note that I've bumped the version in package.json in this commit but I can remove if you prefer.

Copy link
Contributor

@Shengming-Yan Shengming-Yan Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I reckon when customer refer to zaf_sdk in their app, it is using the release version now. So this version is unrelavant unless we can manually keep align with the release version.

With regarding to shall we keep this, as reviewing the purpose of this PR, wonder do you need this info for internal debug only? In that case, probably considering adding x-zendesk-app-framework-sdk-version header for this instead of chain the existing user agent? As all x-zendesk- headers will be filtered(unless in allowlist) out before zendesk proxy dispatch the request, so it will have zero customer impact and in the meantime, internal can still track the zaf-sdk version in the log

@Shengming-Yan Shengming-Yan added the patch when merged bump patch version label Nov 6, 2024
@token-cjg
Bump version, rename method, add tests
23e6df8 
- Rename to updateUserAgentWithAppId
- Bump version
- Add tests for updateUserAgentWithAppId
@token-cjg
Test updateUserAgentWithAppId more thoroughly + break early if appId …
1c88245 
…undefined
@token-cjg token-cjg force-pushed the cgoddard/red-2506/add-user-agent branch from 7fab0d4 to 1c88245 Compare November 6, 2024 08:41
@token-cjg token-cjg changed the title [RED-2503][RED-2506][RED-2507] Add zaf_sdk user agent to headers for requests [RED-2503][RED-2506][RED-2507][RED-2509] Add zaf_sdk user agent to headers for requests Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Shengming-Yan Shengming-Yan Shengming-Yan left review comments

@caspersg caspersg caspersg left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
patch when merged bump patch version
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@token-cjg @caspersg @Shengming-Yan