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

Users under control group are significantly more than users under enabled group #6400

Closed
karrtikr opened this issue Jul 1, 2019 · 1 comment
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug important Issue identified as high-priority

Comments

@karrtikr
Copy link

karrtikr commented Jul 1, 2019

Almost all users seem to fall under control group with current experimental setup.

[
    {
        "name": "AlwaysDisplayTestExplorer - experiment",
        "salt": "AlwaysDisplayTestExplorer",
        "min": 80,
        "max": 100
    },
    {
        "name": "AlwaysDisplayTestExplorer - control",
        "salt": "AlwaysDisplayTestExplorer",
        "min": 0,
        "max": 20
    }
]

Logic for checking if you are in an experiment: In Experiment(foo) =HASH>=min and < max.

Probable Bug : We expect to get HASH from CryptoUtils, which apparently is always returning 0 as hash value, so users always fall under AlwaysDisplayTestExplorer - control group.

export class CryptoUtils implements ICryptoUtils {
    public createHash<E extends keyof IHashFormat>(data: string, encoding: HexBase64Latin1Encoding, hashFormat: E): IHashFormat[E] {
        const hash = createHash('sha512').update(data).digest(encoding);
        return hashFormat === 'number' ? parseInt(hash, undefined) : hash as any;
    }
}

In the code, parseInt is being used incorrectly. Pass a second parameter, 16 to it instead of passing undefined, so it correctly parses hex encoded string to int.

@karrtikr karrtikr added bug Issue identified by VS Code Team member as probable bug needs PR labels Jul 1, 2019
@karrtikr karrtikr added this to the 2019 - June Sprint 13 milestone Jul 1, 2019
@brettcannon brettcannon added reason-new feature important Issue identified as high-priority labels Jul 2, 2019
@kimadeline kimadeline self-assigned this Jul 4, 2019
@kimadeline
Copy link

Todo

  • do that work from the release branch
  • parseInt(hash, 16)
  • unit tests

@karrtikr karrtikr closed this as completed Jul 9, 2019
@ghost ghost removed the needs PR label Jul 9, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jul 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug important Issue identified as high-priority
Projects
None yet
Development

No branches or pull requests

3 participants