Skip to content

Integrate the ExP framework #260

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
May 15, 2020
Merged

Integrate the ExP framework #260

merged 5 commits into from
May 15, 2020

Conversation

jdneo
Copy link
Member

@jdneo jdneo commented May 11, 2020

No description provided.

@jdneo jdneo added this to the 0.10.1 milestone May 11, 2020
daytonellwanger
daytonellwanger reviewed May 11, 2020
View reviewed changes
package.json Outdated
"webpack-cli": "^3.3.11"
},
"dependencies": {
"fs-extra": "^7.0.1",
"lodash": "^4.17.15",
"minimatch": "^3.0.4",
"vscode-extension-telemetry-wrapper": "^0.7.1"
"vscode-extension-telemetry-wrapper": "^0.7.1",
"vscode-tas-client": "0.0.702"
Copy link

Choose a reason for hiding this comment

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

You should actually use version 0.0.659 of vscode-tas-client for now. 0.0.702 has a bug that I'll resolve later this week.

src/ExpManager.ts Outdated
return false;
}

try {
Copy link

Choose a reason for hiding this comment

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

The vscode-tas-client package will query the treatment assignment service in the background (every 30 minutes), so flight information should never be much staler than the last opened instance of VS Code. If you want to be absolutely sure your extension has up-to-date flight information, you can do this. However, it probably isn't necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

So does that mean that we can just call isCachedFlightEnabled() here, and call isFlightEnabledAsync() for 1 time in the init method?

Copy link

Choose a reason for hiding this comment

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

You shouldn't ever need to call isFlightEnabledAsync unless you want the absolute latest flight information. But I can't think of any scenarios where you would.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. Thank you @daytonellwanger.

Another question, is below code needed in the init functions?

const asyncDummyCheck = (arg: any) => {
        expService?.isFlightEnabledAsync('dummy').then((v) => { return; }).catch((r) => { return; });
    };
    expService?.isCachedFlightEnabled('dummy').then(asyncDummyCheck).catch(asyncDummyCheck);
}

Copy link

Choose a reason for hiding this comment

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

This should no longer be needed with the version of the vscode-tas-client package I mentioned above.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. Thank you for your help @daytonellwanger. :)

I'll update the code according to the comments.

src/ExperimentationService.ts Outdated

export function init(context: vscode.ExtensionContext) {
const extensionName = "vscjava.vscode-java-dependency";
const extensionVersion = "0.10.1";
Copy link

Choose a reason for hiding this comment

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

It might be better to read this from your package.json so you don't have to remember to update this when you change the extension version.

src/controllers/projectController.ts Outdated
@@ -31,10 +32,16 @@ export class ProjectController implements Disposable {
detail: "A project without any build tools",
}];
if (contextManager.getContextValue(Context.MAVEN_ENABLED)) {
projectKinds.push({
const isMavenDefault: boolean = await expManager.isFlightEnabled("defaultMaven");
Copy link

Choose a reason for hiding this comment

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

Are you OK paying the async cost here? Given that this could result in an http request, this could be 1-5 seconds.

@jdneo jdneo requested a review from daytonellwanger May 12, 2020 13:48
daytonellwanger
daytonellwanger reviewed May 12, 2020
View reviewed changes
src/ExperimentationService.ts Outdated
}

export async function init(context: vscode.ExtensionContext): Promise<void> {
const packageJson: {} = await fse.readJSON(context.asAbsolutePath("package.json"));
Copy link

Choose a reason for hiding this comment

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

You might be able to just do require('package.json')?

src/ExperimentationService.ts
expService = getExperimentationService(extensionName, extensionVersion,
TargetPopulation.Public, new ExperimentationTelemetry(), context.globalState);

// Due to a bug in the tas-client module, a call to isFlightEnabledAsync is required to begin
Copy link

Choose a reason for hiding this comment

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

You should be able to drop these dummy checks

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to remove these lines. But I found that when I'm calling getExpService().isCachedFlightEnabled("defaultMaven");, the cached features are empty.

image

Within the dummy code, the cached features are correct.

// The vscode-tas-client I'm using is 0.0.659

Copy link

Choose a reason for hiding this comment

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

Oh interesting. Maybe we still have another bug to track down. OK, keep these checks for now and I'll look into the issue ASAP, and with the next update to the package, you can remove them.

@jdneo
Copy link
Member Author

jdneo commented May 14, 2020

Hey @daytonellwanger，

I have updated the PR. Please take a look when you have time. Thanks a lot.

daytonellwanger
daytonellwanger previously approved these changes May 15, 2020
View reviewed changes
src/controllers/projectController.ts Outdated
@@ -31,10 +32,16 @@ export class ProjectController implements Disposable {
detail: "A project without any build tools",
}];
if (contextManager.getContextValue(Context.MAVEN_ENABLED)) {
projectKinds.push({
const isMavenDefault: boolean = await getExpService().isCachedFlightEnabled("defaultMaven");
Copy link

Choose a reason for hiding this comment

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

null check after getExpService?

@jdneo jdneo modified the milestones: 0.10.1, 0.10.2 May 15, 2020
daytonellwanger
daytonellwanger approved these changes May 15, 2020
View reviewed changes
Copy link

@daytonellwanger daytonellwanger left a comment

Choose a reason for hiding this comment

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

LGTM!

@jdneo jdneo merged commit 875c9c5 into master May 15, 2020
@jdneo jdneo deleted the cs/test branch May 15, 2020 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Eskibear Eskibear Eskibear approved these changes

@daytonellwanger daytonellwanger daytonellwanger approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.10.2
Development

Successfully merging this pull request may close these issues.
3 participants
@jdneo @Eskibear @daytonellwanger