AtlasEngine: Remove experimental tag and add tracing #13939
Conversation
lhecker
added
Area-Settings
lhecker
changed the title
lhecker
force-pushed
the
dev/lhecker/atlas-engine-settings
branch
2 times, most recently
from
51d6a44 to
f8a5b9c
Compare
src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw
Outdated
Show resolved
Hide resolved
| const auto& globals = settings->_globals; | ||
| TraceLoggingWrite( | ||
| g_hSettingsModelProvider, | ||
| "AtlasEngine_Enabled", |
There was a problem hiding this comment.
This is sorta what we were talking about. This is a usage event (user initiated) emitted for something Terminal did (load its settings). Hmm.
| TraceLoggingBool(globals->HasUseAtlasEngine(), "UseAtlasEngineOverridden", "true if useAtlasEngine is set"), | ||
| TraceLoggingBool(globals->UseAtlasEngine(), "UseAtlasEngine", "true if AtlasEngine is enabled"), | ||
| TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES), | ||
| TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage)); |
There was a problem hiding this comment.
In team sync you mentioned a bunch of performance data we wanted to get insights into as well. Atlas resizing, etc.
Did you want to include those in this PR as well?
There was a problem hiding this comment.
I'd have to figure out how to implement them first and I felt like getting this crucial change in ASAP is much more important than that.
There was a problem hiding this comment.
I agree with Leonard on this one. Let's start with simple
lhecker
force-pushed
the
dev/lhecker/atlas-engine-settings
branch
from
9a9b411 to
8f4997b
Compare
|
I'm okay with the setting moving in the UI to Rendering where it belongs. I do think its logical place makes no sense today. However, its physical place can remain in |
|
The thing about having a different logical place is that we could have multiple logical places for it. the Rendering page can set the defaults one, but what if you have a profile that overrides it? I don't know. |
|
The latest commits removes the "compatibility" prefix and allows the setting to be changed in both places (rendering and advanced default profile settings). |
| "AtlasEngine_Usage", | ||
| TraceLoggingDescription("Event emitted upon settings load, containing the number of profiles opted-in/out of useAtlasEngine"), | ||
| TraceLoggingUIntPtr(enabled[0], "UseAtlasEngineDisabled", "Number of profiles for which AtlasEngine is disabled"), | ||
| TraceLoggingUIntPtr(enabled[1], "UseAtlasEngineEnabled", "Number of profiles for which AtlasEngine is enabled"), |
There was a problem hiding this comment.
Do we maybe want to break this down further?
- Users that don't have the setting at all (
!HasUseAtlasEngineDisabled()) - Users that explicitly opted in
- Users that explicitly opted out
and maybe a total number of profiles too? Trying to figure out how many folks actually just set this in profiles.defaults might complicate this...
There was a problem hiding this comment.
We can infer all the first 3 points from the version number alone...
But I think I just realized that might not be added by default to the event data, right? In that case you're right, I'll have to add something like that.
Since the number of profiles is UseAtlasEngineDisabled + UseAtlasEngineEnabled I think we can infer that if needed for sure.
There was a problem hiding this comment.
Ah right, we can infer that the user opted out if Disabled > 0. The version is part of the data we automatically get.
There was a problem hiding this comment.
Oh so I guess (Num Preview users)-(preview users with it disabled)=(preview users with it enabled, either by default or explicitly). Okay, that makes sense.
zadjii-msft
added
the
AutoMerge
|
Hello @zadjii-msft! Because this pull request has the 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 (
|
ghost
merged commit
|
🎉 Handy links: |
With AtlasEngine being enabled by default in 1.16 Preview it would look weird if the `useAtlasEngine` option would still be called "experimental". Additionally we're interested in how many users opt out of useAtlasEngine, indicating major issues that would require us to disable it by default again. Related to #13936 * Toggling `useAtlasEngine` works as expected ✅ * Observe new event with TVPP ✅ (cherry picked from commit 6816620)
With AtlasEngine being enabled by default in 1.16 Preview it would look weird
if the
useAtlasEngineoption would still be called "experimental".Additionally we're interested in how many users opt out of useAtlasEngine,
indicating major issues that would require us to disable it by default again.
Related to #13936
Validation Steps Performed
useAtlasEngineworks as expected ✅