-
Notifications
You must be signed in to change notification settings - Fork 8.7k
Add telemetry for settings UI traffic #19156
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
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
You'll need to add one! Crib |
"AddNewProfile", | ||
TraceLoggingDescription("Event emitted when the user adds a new profile"), | ||
TraceLoggingValue("Duplicate", "Type", "The type of the creation method (i.e. empty profile, duplicate)"), | ||
TraceLoggingValue(to_hstring(profileGuid).c_str(), "SourceGuid", "The guid of the profile that was copied"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't do that - use TraceLoggingGuid
instead and cast profileGuid
to GUID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, is the source GUID meaningful? unless it's one of the ones we know about it's just noise.
may be worth checking the source source to see if people are cloning autogenerated or fragment profiles. not to leak the source, just a boolean confirming whether Source was not empty.
@@ -26,6 +26,8 @@ namespace winrt | |||
|
|||
namespace winrt::Microsoft::Terminal::Settings::Editor::implementation | |||
{ | |||
static constexpr std::wstring_view ColorSchemesPageId{ L"page.colorSchemes" }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit about the names: NavigatedToPage is an unambiguous namespace, right? so the page.
part of page.foo
is redundant
@@ -26,6 +26,8 @@ namespace winrt | |||
|
|||
namespace winrt::Microsoft::Terminal::Settings::Editor::implementation | |||
{ | |||
static constexpr std::wstring_view ColorSchemesPageId{ L"page.colorSchemes" }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you're only going to use these string views in one place, you could just inline the string into the telemetry event
const auto& currentPkgVM = _ViewModel.CurrentExtensionPackage(); | ||
const auto& currentPkg = currentPkgVM.Package(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const auto& currentPkgVM = _ViewModel.CurrentExtensionPackage(); | |
const auto& currentPkg = currentPkgVM.Package(); | |
const auto currentPkgVM = _ViewModel.CurrentExtensionPackage(); | |
const auto currentPkg = currentPkgVM.Package(); |
@@ -41,6 +42,36 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation | |||
vmImpl->ExtensionPackageIdentifierTemplateSelector(_extensionPackageIdentifierTemplateSelector); | |||
vmImpl->LazyLoadExtensions(); | |||
vmImpl->MarkAsVisited(); | |||
|
|||
if (_ViewModel.IsExtensionView()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for consistency, here and below, go through vmImpl->
. no point in calling the projected wrapper methods when you have the literal implementation type right here
TraceLoggingDescription("Event emitted when the user navigates to a page in the settings UI"), | ||
TraceLoggingValue(ProfilesAppearancePageId.data(), "PageId", "The identifier of the page that was navigated to"), | ||
TraceLoggingValue(_Profile.IsBaseLayer(), "IsProfileDefaults", "If the modified profile is the profile.defaults object"), | ||
TraceLoggingValue(to_hstring(_Profile.Guid()).c_str(), "ProfileGuid", "The guid of the profile that was navigated to"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same note about guid
"CreateUnfocusedAppearance", | ||
TraceLoggingDescription("Event emitted when the user creates an unfocused appearance for a profile"), | ||
TraceLoggingValue(_Profile.IsBaseLayer(), "IsProfileDefaults", "If the modified profile is the profile.defaults object"), | ||
TraceLoggingValue(to_hstring(_Profile.Guid()).c_str(), "ProfileGuid", "The guid of the profile that was navigated to"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same note about guid
TraceLoggingValue(_Profile.IsBaseLayer(), "IsProfileDefaults", "If the modified profile is the profile.defaults object"), | ||
TraceLoggingValue(to_hstring(_Profile.Guid()).c_str(), "ProfileGuid", "The guid of the profile that was navigated to"), | ||
TraceLoggingValue(_Profile.Source().c_str(), "ProfileSource", "The source of the profile that was navigated to"), | ||
TraceLoggingValue(false, "Orphaned", "Tracks if the profile was orphaned"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one could argue that the orphaned profile page is just a different type of page.
TraceLoggingDescription("Event emitted when the user navigates to a page in the settings UI"), | ||
TraceLoggingValue(ProfilesBasePageId.data(), "PageId", "The identifier of the page that was navigated to"), | ||
TraceLoggingValue(_Profile.IsBaseLayer(), "IsProfileDefaults", "If the modified profile is the profile.defaults object"), | ||
TraceLoggingValue(to_hstring(_Profile.Guid()).c_str(), "ProfileGuid", "The guid of the profile that was navigated to"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same note about guid
"NavigatedToPage", | ||
TraceLoggingDescription("Event emitted when the user navigates to a page in the settings UI"), | ||
TraceLoggingValue(ProfilesBaseOrphanedPageId.data(), "PageId", "The identifier of the page that was navigated to"), | ||
TraceLoggingValue(_Profile.IsBaseLayer(), "IsProfileDefaults", "If the modified profile is the profile.defaults object"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is an impossibility
Summary of the Pull Request
Adds a telemetry provider to the Terminal.Settings.Editor project as well as new telemetry events to track traffic through the settings UI. Specifically, the following events were added:
NavigatedToPage
: Event emitted when the user navigates to a page in the settings UIPageId
parameter that includes the identifier of the page that was navigated topage.editColorScheme
)SchemeName
parameter tracks the name of the color scheme that's being editedpage.extensions
:ExtensionPackageCount
: The number of extension packages displayedProfilesModifiedCount
: The number of profiles modified by enabled extensionsProfilesAddedCount
: The number of profiles added by enabled extensionsColorSchemesAddedCount
: The number of color schemes added by enabled extensionspage.extensions.extensionView
:FragmentSource
: The source of the fragment included in this extension packageFragmentCount
: The number of fragments included in this extension packageEnabled
: The enabled status of the extensionpage.newTabMenu
) if the page is representing a folder viewpage.profile.*
:IsProfileDefaults
: if the modified profile is the profile.defaults objectProfileGuid
: the guid of the profile that was navigated toProfileSource
: the source of the profile that was navigated topage.profile
(aka the base profile page):Orphaned
: tracks if the profile was orphanedHidden
: tracks if the profile is hiddenpage.profile.appearance
)HasBackgroundImage
:if the profile has a background image defined
page.profile.appearance
)HasUnfocusedAppearance
:if the profile has an unfocused appearance defined
AddNewProfile
: Event emitted when the user adds a new profileIsExtensionView
parameter tracks if the page is representing a view of an extensionType
parameter that represents the type of the creation method (i.e. empty profile, duplicate)ResetApplicationState
: Event emitted when the user resets their application state (via the UI)ResetToDefaultSettings
: Event emitted when the user resets their settings to their default value (via the UI)OpenJson
: Event emitted when the user clicks the Open JSON button in the settings UISettingsTarget
parameter that represents the target settings file (i.e. settings.json vs defaults.json)CreateUnfocusedAppearance
: Event emitted when the user creates an unfocused appearance for a profileIsProfileDefaults
: if the modified profile is the profile.defaults objectProfileGuid
: the guid of the profile that was navigated toProfileSource
: the source of the profile that was navigated toDeleteProfile
: Event emitted when the user deletes a profileProfileGuid
,ProfileSource
,Orphaned
from theNavigatedToPage
section aboveThe page ids can be reused later as a serialized reference to the page. We already use the one for the extensions page for the "new" badge.