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.

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

Track js compilation time #9794

Merged
merged 5 commits into from
Dec 12, 2023
Merged

Track js compilation time #9794

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
38638ce
Keep track of compilation times
aznhassan Dec 6, 2023
f9ce4be
Enable telemetry in runner page
aznhassan Dec 6, 2023
d4fa7e4
Change duration to durationMs
aznhassan Dec 6, 2023
a25e584
Format added lines
aznhassan Dec 6, 2023
b3053c9
Use single event with operatin name to track peformance
aznhassan Dec 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion pxtrunner/runner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,7 @@ namespace pxt.runner {

function initInnerAsync() {
pxt.setAppTarget((window as any).pxtTargetBundle)
pxt.analytics.enable(pxt.Util.userLanguage());
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about this change? We have other ticks coming in so feels unexpected to have to change this, enableAnalytics call should be getting in thru here https://github.com/search?q=repo%3Amicrosoft%2Fpxt%20analytics.enable&type=code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I don't include this line, the tickEvents do not make it to Application Insights. See how there are only share.isMultiplayer ticks most recently where I commented out this line? Whereas in around 3:04pm I had still had this line enabled and we get the same events mixed in with the new runner events.

Suggested change
pxt.analytics.enable(pxt.Util.userLanguage());
pxt.analytics.enable(pxt.Util.userLanguage());

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding @thsparks. I have observed that pageview ticks stopped flowing recently as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jwunderl There are five files where pxt.analytics.enable are called in the linked you provided:

  1. Kiosk does not apply in this case since I tested it by opening a share-page after running Arcade locally.
  2. I don't believe the share page loads the web app, so telemetry should not be enabled via that route. Though if I'm incorrect, please let me know!
  3. Multiplayer page separately enables telemetry, but that does not apply here.
  4. Same with the teachertool page.

I can see that argument that if do enable additional telemetry for share pages, then we should match what we've done before in these other examples that you've linked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding @thsparks. I have observed that pageview ticks stopped flowing recently as well.

These changes are unrelated to tracking page views, which are making it. @abchatra

Util.assert(!!pxt.appTarget);

const href = window.location.href;
Expand Down Expand Up @@ -468,7 +469,13 @@ namespace pxt.runner {

export async function fetchSimJsInfo(simOptions: SimulateOptions): Promise<pxtc.BuiltSimJsInfo> {
try {
return await pxt.Cloud.downloadBuiltSimJsInfoAsync(simOptions.id);
const start = Date.now();
const result = await pxt.Cloud.downloadBuiltSimJsInfoAsync(simOptions.id);
pxt.tickEvent("perfMeasurement", {
durationMs: Date.now() - start,
operation: "fetchSimJsInfo",
});
return result;
} catch (e) {
// This exception will happen in the majority of cases, so we don't want to log it unless for debugging.
pxt.debug(e.toString());
Expand All @@ -477,6 +484,7 @@ namespace pxt.runner {
}

export async function buildSimJsInfo(simOptions: SimulateOptions): Promise<pxtc.BuiltSimJsInfo> {
const start = Date.now();
await loadPackageAsync(simOptions.id, simOptions.code, simOptions.dependencies);

let didUpgrade = false;
Expand Down Expand Up @@ -539,6 +547,10 @@ namespace pxt.runner {

const res = pxtc.buildSimJsInfo(compileResult);
res.parts = compileResult.usedParts;
pxt.tickEvent("perfMeasurement", {
durationMs: Date.now() - start,
operation: "buildSimJsInfo",
});
return res;
}

Expand Down
Loading