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.

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

bug(settings): Backend events for login_reg and login_complete have gone missing #18059

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dschom
Copy link
Contributor

@dschom dschom commented Nov 23, 2024

Because

  • Some back end events seemed to disappear
  • This is due to the entry point not being defined on the event

This pull request

  • Fixes missing entrypoint glean data
  • Ensures entrypoint is sent in graphql requests if it is present in URL
  • Fixes typo in utm parameter. utmContext should have been utmContent.
  • Puts the MetricContext model in a shared place to reduce the chance this happens again.

Issue that this pull request solves

Closes: # (issue number)

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

Copy link
Contributor

@chenba chenba left a comment

Choose a reason for hiding this comment

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

I don't see the entrypoint related changes. Did you forget to add some file to the commit?

libs/shared/metrics/glean/src/lib/metrics.context.ts Outdated Show resolved Hide resolved
packages/fxa-auth-client/lib/client.ts Show resolved Hide resolved
@@ -118,23 +119,6 @@ export type AuthServerError = Error & {
retryAfterLocalized?: string;
};

export const MetricsContextKeys = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chenba, This collection was the issue. It didn't contain entrypoint.

*/
export class MetricsContext {
deviceId?: string;
entrypoint?: string;
Copy link
Contributor Author

@dschom dschom Nov 25, 2024

Choose a reason for hiding this comment

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

@chenba entrypoint is now a valid 'key' for Metricscontext. With this addition, the query param and is now copied into the metrics context that is sent to the server.

@dschom
Copy link
Contributor Author

dschom commented Nov 25, 2024

I don't see the entrypoint related changes. Did you forget to add some file to the commit?

@chenba Because of the refactor the change isn't totally obvious. See this comment and this comment.

Copy link
Contributor

@chenba chenba left a comment

Choose a reason for hiding this comment

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

👍 Thanks

…one missing

- Some back end events seemed to disappear
- This is due to the entry point not being defined on the event

- Fixes missing entrypoint glean data
- Ensures entrypoint is sent in graphql requests if it is present in URL
- Fixes typo in utm parameter. `utmContext` should have been `utmContent`.
- Puts the `MetricContext` model in a shared place to reduce the chance this happens again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants