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

feat(glean): add is_baseline metric to pings #9476

Merged
merged 3 commits into from
Sep 1, 2023
Merged

Conversation

LeoMcA
Copy link
Member

@LeoMcA LeoMcA commented Aug 10, 2023

Summary

https://mozilla-hub.atlassian.net/browse/MP-542
https://bugzilla.mozilla.org/show_bug.cgi?id=1848187

Problem

While we could track page views with a baseline status by manually exporting a list of pages, and feeding that into our metrics tools, this doesn't scale very well.

Solution

Add an is_baseline metric to page views and events:

  • null - no baseline status on the page
  • true - the page is baseline
  • false - the page is not baseline

Also update the CSP to allow sending glean pings from localhost:5042


How did you test this change?

set:

REACT_APP_GLEAN_ENABLED=true
REACT_APP_GLEAN_DEBUG=true
REACT_APP_DEV_MODE=true

run: yarn && yarn dev

to test the null case:

to test the true case:

to test the false case:

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@LeoMcA LeoMcA marked this pull request as ready for review August 11, 2023 10:36
@LeoMcA LeoMcA requested a review from fiji-flo August 11, 2023 10:36
Copy link
Contributor

@fiji-flo fiji-flo left a comment

Choose a reason for hiding this comment

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

@LeoMcA: Code looks fine. Should we go for a string instead of a bool. It seems like baseline will not really be binary.

@github-actions github-actions bot added the merge conflicts 🚧 Please rebase onto or merge the latest main. label Aug 29, 2023
@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@LeoMcA
Copy link
Member Author

LeoMcA commented Aug 29, 2023

Should we go for a string instead of a bool. It seems like baseline will not really be binary.

True, I ummed and ahhed over this - while baseline itself will still be boolean, we will probably be exposing more of a ternary to users (baseline, nearly baseline, not baseline), and I think there's value in measuring what that status we surface to users is. I'll make the change.

@github-actions github-actions bot removed the merge conflicts 🚧 Please rebase onto or merge the latest main. label Aug 29, 2023
@LeoMcA LeoMcA requested a review from fiji-flo August 29, 2023 17:16
Copy link
Contributor

@fiji-flo fiji-flo left a comment

Choose a reason for hiding this comment

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

Looks good! One nit if even.

Comment on lines +200 to +205
isBaseline:
doc?.baseline?.is_baseline === undefined
? undefined
: doc.baseline.is_baseline
? "baseline"
: "not_baseline",
Copy link
Contributor

Choose a reason for hiding this comment

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

some potential JS magic:

isBaseline: { undefined: undefined, true: "baseline", false: "no_baseline"}[doc?.baseline?.is_baseline]

or

isBaseline: { true: "baseline", false: "no_baseline"}[doc?.baseline?.is_baseline]

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately typescript makes this less fun, as I have to manually cast doc?.baseline?.is_baseline to a string:

      isBaseline: { true: "baseline", false: "no_baseline" }[
        String(doc?.baseline?.is_baseline)
      ],

So I'm going to leave as it is, especially since I imagine we'll need a second layer of logic for future the almost-baseline case, but I'll remember this for the future!

@LeoMcA LeoMcA merged commit 231d6aa into main Sep 1, 2023
@LeoMcA LeoMcA deleted the is-baseline-metric branch September 1, 2023 16:01
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.

None yet

2 participants