-
-
Notifications
You must be signed in to change notification settings - Fork 256
feat: add @metamask/profile-metrics-controller package with ProfileMetricsService
#7194
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
Conversation
522b8f7 to
747f8f7
Compare
@metamask/user-profile-controller package@metamask/user-profile-controller package with UserProfileService
|
@mikesposito Do we plan to add a controller to this newly added package ? |
|
@cryptodev-2s Yes, it is being added in this follow-up PR: #7196 (the controller consumes the service added here) |
@metamask/user-profile-controller package with UserProfileService@metamask/profile-metrics-controller package with ProfileMetricsService
mathieuartu
left a comment
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.
Overall looks good, left a couple of comments re how we intertwine with @metamask/profile-sync-controller. LMK what you think 🙂
| messenger, | ||
| fetch: fetchFunction, | ||
| policyOptions = {}, | ||
| env = Env.DEV, |
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.
Should we default to Env.PRD since AuthenticationController defaults to this value as well?
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.
I'm a little conflicted about this one. True that it must be the same as AuthenticationController, but on the other hand, having Env.PRD as default and using that everywhere (including local env) doesn't feel right either.
packages/profile-metrics-controller/src/ProfileMetricsService.ts
Outdated
Show resolved
Hide resolved
| messenger, | ||
| fetch: fetchFunction, | ||
| policyOptions = {}, | ||
| env = SDK.Env.DEV, |
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.
Bug: Default environment inconsistent with AuthenticationController
The env parameter defaults to SDK.Env.DEV but AuthenticationController defaults to Env.PRD (production). This inconsistency means that when ProfileMetricsService is instantiated without an explicit env, it will send requests to the development API while using a bearer token obtained from AuthenticationController that's configured for production. This could cause authentication mismatches in production environments where env isn't explicitly set. The PR discussion specifically flagged this concern.
| messenger, | ||
| fetch: fetchFunction, | ||
| policyOptions = {}, | ||
| env = SDK.Env.DEV, |
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.
Bug: Environment default mismatch causes authentication failures
The env parameter defaults to SDK.Env.DEV, but AuthenticationController defaults to Env.PRD. When ProfileMetricsService is instantiated without explicitly specifying env, it will request bearer tokens from a production-configured AuthenticationController but attempt to use them against development API endpoints. This environment mismatch could cause authentication failures since tokens from one environment may not be valid for another. The PR discussion confirms this concern, with acknowledgment that "it must be the same as AuthenticationController."
packages/profile-metrics-controller/src/ProfileMetricsService.ts
Outdated
Show resolved
Hide resolved
mathieuartu
left a comment
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! Nice work!
cryptodev-2s
left a comment
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.
LGTM!
Explanation
The
@metamask/profile-metrics-controllerpackage is being added to the monorepo.ProfileMetricsServiceis also being added to the package, to be used for sending requests to the Authentication API. The service will be consumed byProfileMetricsController, which is being added in this follow-up PR: #7196References
Checklist
Note
Adds new @metamask/profile-metrics-controller with a ProfileMetricsService for submitting metrics to the Auth API, plus tests and repo wiring.
@metamask/profile-metrics-controllerProfileMetricsServicewithsubmitMetricsusingAuthenticationController:getBearerTokenandcreateServicePolicy(retry, degraded threshold, circuit breaker); environment-awaregetAuthUrl.ProfileMetricsService:submitMetrics; exports types and service.entropySourceId, degraded timing, retry limits, circuit break/open/restore.package.json,jest.config.js,tsconfig*,typedoc.json,README,LICENSE,CHANGELOG.README.mdpackage list and dependency graph to includeprofile_metrics_controllerand its deps..github/CODEOWNERSandteams.json.tsconfig.jsonandtsconfig.build.json; updatesyarn.lock.Written by Cursor Bugbot for commit 274982b. This will update automatically on new commits. Configure here.