fix(metrics): DENG-3273 Update event names for consistency between `accounts-events` and `events` pings

akkomar commented 1 month ago

Because

We want event names in accounts-events ping to be consistent with event metric categories and names in events ping

This pull request

Updates recently added event names for consistency

Issue that this pull request solves

Closes: issue described in https://mozilla-hub.atlassian.net/browse/DENG-3273?focusedCommentId=907454

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).

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

Any other information that is important to this pull request.

chenba commented 1 month ago

@akkomar Thanks!

chenba commented 1 month ago

I'm wondering if instead of duplicating the names email (which I think ideally would be email_first), reg, login, and adding them to the front of the name, we should just move them to the front of the name so they only appear once? i.e instead of email_google_oauth_email_first_start it could be email_first_google_oauth_start, etc

Renaming email to email_first would affect the current email.first_view event. I like the idea but I'll defer to @akkomar since he understand the effects of the rename far better than me.

akkomar commented 1 month ago

I'm wondering if instead of duplicating the names email (which I think ideally would be email_first), reg, login, and adding them to the front of the name, we should just move them to the front of the name so they only appear once? i.e instead of email_google_oauth_email_first_start it could be email_first_google_oauth_start, etc

Renaming email to email_first would affect the current email.first_view event. I like the idea but I'll defer to @akkomar since he understand the effects of the rename far better than me.

For context, this PR renames accounts_events events to make them consistent with taxonomy defined in https://github.com/mozilla/fxa/blob/main/packages/fxa-shared/metrics/glean/fxa-ui-metrics.yaml. These definitions should be the source of truth.

google_oauth_email_first_start is under email category. We should not rename the existing email category because this would affect other events that we already collect and use in analysis. I think we can either:

Keep the event under email category and rename it to google_oauth_first_start (or something else), or
Add a new category email_first and put this event there

By looking at descriptions of these events, it seems to me that keeping them all together in the email category makes more sense. The question is how exactly to name them. Renaming these new events is fine since event names are not translated to BQ column names and they have not yet landed in production. We only need to be careful to not modify categories and names of events that we already collect and use for analysis.

ksiegler1 commented 1 month ago

Got it. Pehaps we can keep the category email but still have the event named as email_first_google_oauth_start instead of email_google_oauth_email_first_start? This is the naming convention that would show up in the accounts_frontend database am I understanding correctly?

chenba commented 1 month ago

... By looking at descriptions of these events, it seems to me that keeping them all together in the email category makes more sense. ...

It's partly my fault for not being thorough enough with the code review on the PR that added the 'email' category. Moreover, all the mentions of 'email first' in the descriptions should've been 'email-first'. I'm not suggesting that we should rename an existing category. I just want to provide some context on why @ksiegler1 want to name it 'email_first' and not 'email'.

akkomar commented 1 month ago

Got it. Pehaps we can keep the category email but still have the event named as email_first_google_oauth_start instead of email_google_oauth_email_first_start? This is the naming convention that would show up in the accounts_frontend database am I understanding correctly?

Yes, we can keep the category email and rename the event to first_google_oauth_start. It wil show up accounts_frontend dataset in BigQuery as email.first_google_oauth_start (I'm using dot here to emphasize event metric type, in accounts_events ping this is an underscore).

I can update this PR if that sounds good.

chenba commented 1 month ago

https://github.com/mozilla/fxa/pull/17218 is the continuation of this.

mozilla / fxa