-
Notifications
You must be signed in to change notification settings - Fork 248
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
Simplified onboarding experiment assignment and logging #1036
Conversation
6afa532
to
6dc944f
Compare
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.
Everything seems to work great on my side! Love the simplified steps, too. plus it looks great, and makes it muchhhhh easier for users to onboard! thanks for updating this!
pull_1036.-.test.mp4
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.
Thanks for writing comprehensive unit tests that make reviewing a breeze!
let arm | ||
let excludeFromExperiment | ||
try { | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
const store = JSON.parse(storedSpec) | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access | ||
arm = store?.arm | ||
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | ||
excludeFromExperiment = !!store?.excludeFromExperiment |
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.
What do you think about typing these variables to avoid disabling ESLint rules?
let arm | |
let excludeFromExperiment | |
try { | |
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | |
const store = JSON.parse(storedSpec) | |
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access | |
arm = store?.arm | |
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | |
excludeFromExperiment = !!store?.excludeFromExperiment | |
let arm: OnboardingExperimentArm | undefined | |
let excludeFromExperiment: boolean | undefined | |
try { | |
const store = JSON.parse(storedSpec) as SelectedArm | |
arm = store.arm | |
excludeFromExperiment = !!store.excludeFromExperiment |
Keeping in mind that we are inside the try
block, it should be safe.
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.
@valerybugakov The root of the problem isn't the types of arm
and excludeFromExperiment
, but that the JSON result is basically untyped. The as SelectedArm
you have there is an assertion grounded in optimism about local storage, but I've learned to be very pessimistic about client storage 😅 . So I'd like to keep the lint suppressions because I think it's says "DANGER!" more obviously than the as
...
I'm going to land-this as-is but I'm 100% open to changing this in a follow-up. I just need more convincing.
My IDEAL situation, we would have a framework which could take untyped JSON, apply a schema to it, and give us typed results OR a consistent set of error messages instead of this tedious hand parsing.
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 agree that it's nice to highlight the danger of the local storage being corrupted. However, I would consider this case safe because we operate in the try
block (we "try" to type-cast) and already explicitly throw an error saying the data has an unexpected format. I like the as
casting here because it gives type suggestions later. WDYT?
Also, typing let arm
and let excludeFromExperiment
helps us understand the expected values even if we keep eslint comments.
My IDEAL situation, we would have a framework which could take untyped JSON, apply a schema to it
Same. Would love to apply it here.
} | ||
|
||
export function pickArm(useThisTelemetryService: TelemetryService): OnboardingExperimentArm { | ||
telemetryService = useThisTelemetryService |
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.
Do you know if there's a reason for us to pass the telemetrySerice
through our abstractions from the top level instead of importing it from the module level? Importing it directly would simplify the way we mock it.
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.
Agree this is a wart. In general I'm a fan of passing dependencies over globals for various reasons, but in this codebase we have tons of globals and I don't see why TelemetryService should be different! @akalia25, do you have an opinion?
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.
TelemetryService lives through the shared
abstraction veil, maybe there's a client who can't boot it up very quickly. But it's always possible to buffer messages for a while and flush them later...
26c93be
to
daf9101
Compare
Users are randomly assigned to get the classic onboarding experience ('control') or a simplified onboarding experience without app setup ('treatment'.) This allocation is hard-coded in the extension; if we want a different rollout we need to update the extension.
Once we render the login component, we consider the user has been exposed to a specific arm of the trial and we cache this fact in local storage.
Currently fixes the rollout at 0.5 so people running main will get exposed to the experiment. We can bump
SIMPLIFIED_ARM_ALLOCATION
back to zero if we want to hold back the experiment.Testers can set a boolean property,
testing.simplified-onboarding
, to force the extension to display the treatment (true) or control (false) arm of the trial.Part of #632
Test plan
Automated test:
Manual test:
"testing.simplified-onboarding": true
. Reload VScode.