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

Add build and launch telemetry #594

Merged
merged 3 commits into from
Oct 8, 2024
Merged

Add build and launch telemetry #594

merged 3 commits into from
Oct 8, 2024

Conversation

kmagiera
Copy link
Member

@kmagiera kmagiera commented Oct 7, 2024

This PR adds more loggind and telemetry around build and launch process.

Building and launching the app is one of the most fragile element of the IDE pipeline and we get numerous reports around that. While there are a few known issues around it that we are fixing and investigating. I figured it'd be good to also add telemetry around that such that we can track how it works for a larger group of people and also be able to detect regressions.

Test plan

Just run the example app with clean build and make sure nothing breaks.

Copy link

vercel bot commented Oct 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
radon-ide ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 8, 2024 9:00am

@@ -100,6 +101,11 @@ export class DeviceSession implements Disposable {
}

private async launchApp() {
const launchReequestTime = Date.now();
Copy link
Contributor

Choose a reason for hiding this comment

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

typo:
launchReequestTime -> launchRequestTime

Comment on lines +128 to 131
waitForAppReady.then(() => clearTimeout(reportWaitingStuck));
}

const [previewUrl] = await Promise.all([this.device.startPreview(), waitForAppReady]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we wait for waitForAppReady once?

Suggested change
waitForAppReady.then(() => clearTimeout(reportWaitingStuck));
}
const [previewUrl] = await Promise.all([this.device.startPreview(), waitForAppReady]);
}
const [previewUrl] = await Promise.all([this.device.startPreview(), waitForAppReady]);
if (shouldWaitForAppLaunch) {
clearTimeout(reportWaitingStuck);
}

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 like my approach moare as it doesn't mix the reporting code with the existing logic. Whatever is placed inside that if block I added, is just for reporting and can be deleted. Otherwise I'd have to keep the reportWaitingStuck in main scope, while we'd need to have shouldWaitForAppLaunch checks both before and after the await.

On top of that this isn't really an equivalent, as the await is for both startPreview and app ready event.

const [previewUrl] = await Promise.all([this.device.startPreview(), waitForAppReady]);
Logger.debug("App and preview ready, moving on...");
this.eventDelegate.onStateChange(StartupMessage.AttachingDebugger);
await this.startDebugger();

const launchDurationSec = (Date.now() - launchReequestTime) / 1000;
Copy link
Contributor

Choose a reason for hiding this comment

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

typo:
launchReequestTime -> launchRequestTime

Copy link
Contributor

@p-malecki p-malecki left a comment

Choose a reason for hiding this comment

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

Left some comments inline

@kmagiera kmagiera merged commit 603b351 into main Oct 8, 2024
2 of 3 checks passed
@kmagiera kmagiera deleted the kmagiera/moar-telemetry branch October 8, 2024 09:00
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.

2 participants