Skip to content

Conversation

@lhecker
Copy link
Member

@lhecker lhecker commented Sep 8, 2022

This commit reduces the amount of telemetry during general usage by about half.
8 events that weren't really used anymore were removed.
1 new event was added ("AppInitialized") which will help us investigate #5907.
During review 9 events were found that were incorrectly tagged as perf. data.

Validation Steps Performed

  • Launch Windows Terminal
  • The "latency" field "AppInitialized" matches the approx. launch time ✅

@lhecker lhecker added Product-Terminal The new Windows Terminal. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Priority-1 A description (P1) Gathering-Data We'd like to gather telemetry to influence our decision labels Sep 8, 2022
@lhecker lhecker added this to the Terminal v1.16 milestone Sep 8, 2022
@lhecker lhecker force-pushed the dev/lhecker/telemetry branch from 3868bd0 to ac18870 Compare September 8, 2022 22:53

GetSystemTimeAsFileTime(&now);

const auto latency = asSeconds(asInteger(now) - asInteger(creationTime));
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand; is it a guarantee that this thread is the right one?

Copy link
Member Author

Choose a reason for hiding this comment

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

The Initialized callback is on the main thread right? I mean after all it calls all those _root, etc. functions... Then yeah it's the right one.
I chose GetThreadTimes instead of GetProcessTimes, because GetProcessTimes is pretty bad when it comes to performance (intentionally so). To improve runtime performance each thread records its own kernel/user-mode times (because then you don't need to acquire any shared/global locks). But if you call GetProcessTimes it has to acquire a shared/global lock to read and accumulate all those counters. GetThreadTimes doesn't have that issue and delivers the same result if called on the main thread.

@lhecker lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 9, 2022
@ghost
Copy link

ghost commented Sep 9, 2022

Hello @lhecker!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 37c159a into main Sep 9, 2022
@ghost ghost deleted the dev/lhecker/telemetry branch September 9, 2022 13:57
DHowett pushed a commit that referenced this pull request Sep 9, 2022
This commit reduces the amount of telemetry during general usage by about half.
8 events that weren't really used anymore were removed.
1 new event was added ("AppInitialized") which will help us investigate #5907.
During review 9 events were found that were incorrectly tagged as perf. data.

* Launch Windows Terminal
* The "latency" field "AppInitialized" matches the approx. launch time ✅

(cherry picked from commit 37c159a)
Service-Card-Id: 85548958
Service-Version: 1.15
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. AutoMerge Marked for automatic merge by the bot when requirements are met Gathering-Data We'd like to gather telemetry to influence our decision Priority-1 A description (P1) Product-Terminal The new Windows Terminal.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants