-
Notifications
You must be signed in to change notification settings - Fork 212
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(next): glean emit view event #17627
base: main
Are you sure you want to change the base?
Conversation
6681864
to
e83637e
Compare
@@ -75,6 +75,12 @@ STATS_D_CONFIG__HOST= | |||
STATS_D_CONFIG__PORT= | |||
STATS_D_CONFIG__PREFIX= | |||
|
|||
# Glean Config | |||
GLEAN_CONFIG__APPLICATION_ID= | |||
# GLEAN_CONFIG__VERSION= # Set in next.config.js |
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 this not be written as follows instead?:
# Set in next.config.js
GLEAN_CONFIG__VERSION=
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 intention here is to show the developer that this config value exists, but that it's not set in any .env
file, but instead is set in next.config.js
. Leaving the comment on the same line as the config var, IMO more clearly indicates that the comment only applies to this line.
apps/payments/next/lib/metrics/glean/server_events.ts | ||
|
||
# payments-metrics | ||
libs/payments/metrics/src/lib/glean/__generated__/server_events.ts |
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.
apps/payments/next/lib/metrics/glean/server_events.ts
auto-generated when I pulled down the branch.
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.
apps/payments/next/lib/metrics/glean/server_events.ts
was the original location of server_events.ts
so it might just be an artifact from before this PR.
If you delete the folder apps/payments/next/lib/metrics
it should not be automatically created again.
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 was able to start the stack and saw the event logged in payments-next.
I left a comment about a file that auto-generated and not listed in .gitignore
and there are copyright header/import lines that are out of order in a few files. Otherwise, LGTM - thanks Reino!
Because: - Record SubPlat P1 metric view event using glean This commit: - Adds payments-metrics library - Adds PaymentsGleanManager to format events data and CMS data into the required format for Glean events recording. - Adds PaymentsGleanService to handle metrics events emitted by Next.js - Adds the manager and service to the NestApp - Adds new config values to payments-next for Glean reporting - Emit view event from Checkout start page Closes #FXA-10087
e83637e
to
7dd569d
Compare
Because
This pull request
Issue that this pull request solves
Closes: #FXA-10087
Checklist
Put an
x
in the boxes that apply