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

Cirrus has enrolments as an array #5509

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Cirrus has enrolments as an array #5509

wants to merge 8 commits into from

Conversation

Vinnl
Copy link
Collaborator

@Vinnl Vinnl commented Jan 16, 2025

This PR I'm not sure about, and it's probably wrong, but: in MNTOR-3812, @rhelmer showed an example with Enrollments being an array, whereas our code says that it's a single object.

I'm not sure if it actually is, and if it is, I'm not sure why and how we're supposed to handle it - in this PR, I'm just always getting the first element, but that's probably not correct. However, I figured I'd submit this PR to at least have this observation in play somewhere.

@Vinnl Vinnl added the Review: XS Code review time: up to 30min label Jan 16, 2025
@Vinnl Vinnl requested a review from rhelmer January 16, 2025 12:34
@Vinnl Vinnl self-assigned this Jan 16, 2025
Copy link

@rhelmer
Copy link
Collaborator

rhelmer commented Jan 16, 2025

Hm I think you're right, here's the sample that the Nimbus team gave us https://gist.github.com/yashikakhurana/f2fec74b0b6182d03adf957ceedd41b3

@rhelmer
Copy link
Collaborator

rhelmer commented Jan 16, 2025

I'd like to land the other PRs first and see what's going on with stage, this was definitely working (with the CirrusV2 flag enabled) before we deployed to prod. It may have broken in some subtle way, but before the holiday break I confirmed that Glean was sending the data it was getting back and experiments seemed to work. It definitely didn't fail the way prod did yesterday.

So I'd like to understand the current state after things are rolled back before we land, I don't doubt that this is a problem though I just want to understand the sequence of what happened.

@rhelmer
Copy link
Collaborator

rhelmer commented Jan 17, 2025

Hm maybe there is a problem with the documentation... I did test this and it worked as expected (with V2), I'll re-test before we land this. It might be a bug in Cirrus (or its docs).

@rhelmer rhelmer added the 🛑 Do Not Merge Do not merge this PR, even if approved. label Jan 17, 2025
Base automatically changed from cirrus-types to main January 17, 2025 21:00
@rhelmer rhelmer removed the 🛑 Do Not Merge Do not merge this PR, even if approved. label Jan 17, 2025
@rhelmer
Copy link
Collaborator

rhelmer commented Jan 17, 2025

Stage is working now with everything else landed, I flipped the CirrusV2 flag on and we're getting useGlean.ts:63 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'toString') at useGlean.ts:63:55

So I think it's OK to land this one, I think we might need to dig into this one some more though.

@Vinnl
Copy link
Collaborator Author

Vinnl commented Jan 20, 2025

@rhelmer That does indicate that we need to fix this, but I'm still not sure that this PR is the right fix. (Or actually, I'm fairly sure it's not.) I still have this open question for Yashika about how to pass the enrollments to Glean, given that Glean only accepts single enrollments.

@rhelmer rhelmer added the 🛑 Do Not Merge Do not merge this PR, even if approved. label Jan 22, 2025
@rhelmer
Copy link
Collaborator

rhelmer commented Jan 22, 2025

Waiting for an update to Glean.js to support arrays in extra_keys

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: XS Code review time: up to 30min 🛑 Do Not Merge Do not merge this PR, even if approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants