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

Track js compilation time #9794

Merged
merged 5 commits into from
Dec 12, 2023
Merged

Track js compilation time #9794

merged 5 commits into from
Dec 12, 2023

Conversation

aznhassan
Copy link
Contributor

@aznhassan aznhassan commented Dec 8, 2023

Enables telemetry in runner.ts to track the time it takes to either download the compiled JS or download and compile the text. These events will help know the impact the pre-compilation step is having.

Validation

Ran pxt-arcade locally with a local pxt on this branched linked. I then changed the InstrumentationKey in apptracking.html and tracking.html to the PPE app insights instance.

Both these links are the same game just shared twice. For the first one I precompiled the JS to the fetchSimJsInfo event would be recorded. After loading each page five times in a new icognito tab each time here is what I found:

  • buildSimJsInfo had an average of 1463.4 milliseconds, a min of 1171 milliseconds, and a max of 1748 milliseconds.
  • fetchSimJsInfo had an average of 148 milliseconds, a min of 29 milliseconds, a max of 723 milliseconds.

Note: It's important to note that for fetching the pre-compiled JS the initial page load was the longest. After that, the request was cached by the browser. While the request to grab the share page's text is also cached when compiling the JS, the actual compilation still takes time. Regardless, fetching the compressed pre-compiled JS is faster than fetching the text and then compiling client-side.

@aznhassan aznhassan requested a review from a team December 8, 2023 22:33
@aznhassan aznhassan self-assigned this Dec 8, 2023
@@ -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

@aznhassan aznhassan requested a review from thsparks December 11, 2023 21:58
Copy link
Contributor

@thsparks thsparks left a comment

Choose a reason for hiding this comment

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

👍

pxtrunner/runner.ts Outdated Show resolved Hide resolved
@aznhassan aznhassan merged commit aa61926 into master Dec 12, 2023
6 checks passed
@aznhassan aznhassan deleted the aznhassan/track_js_compile_time branch December 12, 2023 00:47
aznhassan added a commit that referenced this pull request Jan 4, 2024
aznhassan added a commit that referenced this pull request Jan 5, 2024
* Before compiling, try to fetch built js from backend (#9778)

* Track js compilation time (#9794)

---------

Co-authored-by: Eric Anderson <[email protected]>
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.

4 participants