-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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(@aws-amplify/ui-components): User agent tracking for UI component packages #4804
feat(@aws-amplify/ui-components): User agent tracking for UI component packages #4804
Conversation
This pull request fixes 1 alert when merging 5e4f37e into 6d1019f - view on LGTM.com fixed alerts:
|
packages/amplify-ui-components/__tests__/__snapshots__/bundle-size.spec.js.snap
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is really straightforward! I need to spend some more time tracking down how the values get from UserAgent.prototype.userAgent
to Platform.userAgent
(or not).
User Agent tracking is so crucial to reporting, are there some tests that exist or that we can add to increase our coverage and protect us as we continue refining it?
@ericclemmons basically the Platform.userAgent is appended to the Cognito request (via UserAgent) during I will definitely add unit tests. Thank you for the feedback. |
This pull request fixes 1 alert when merging f05f0a3 into 6d1019f - view on LGTM.com fixed alerts:
|
I have pushed unit tests for this change. |
This pull request fixes 1 alert when merging f354e47 into 6d1019f - view on LGTM.com fixed alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 Thanks for adding the tests! This helps greatly, especially if we ever come across issues with the duplicate content situation (e.g. @aws-amplify/ui
, @aws-amplify/ui-react
, and @aws-amplify/ui-react-native
all calling appendToCognitoUserAgent
).
I'm in favor of shipping this so we can start getting metrics reported back 👍
In the future, I can see us taking two paths:
-
Embracing a registration pattern, where
get userAgent
computes the string based on what packages have been registered.(Something like https://github.com/aws-amplify/amplify-js/blob/master/packages/core/src/Amplify.ts#L27-L35)
-
Solving for per-request attribution. (possibly via stacktrace or WeakMap's)
For now, let's get visibility we haven't had before! 💪
Not sure why the build is failing though :( |
@ericclemmons |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks Good!
This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs. Looking for a help forum? We recommend joining the Amplify Community Discord server |
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.