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

Jump to bottom

Track and log changes to settings #17678

Merged
merged 16 commits into from
Aug 23, 2024
Merged

Track and log changes to settings #17678

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
6ba3fc1
Track and log changes to settings
carlos-zamora Jul 31, 2024
f4f6f11
log settings model objects
carlos-zamora Aug 2, 2024
19a3dec
fix themes
carlos-zamora Aug 6, 2024
34c93d1
code format
carlos-zamora Aug 6, 2024
d0ba995
remove dead code
carlos-zamora Aug 6, 2024
7bb991f
use strings instead of string_view
carlos-zamora Aug 7, 2024
b026175
use string_views where possible
carlos-zamora Aug 7, 2024
8bda86e
exclude stuff from userDefaults.json
carlos-zamora Aug 12, 2024
8a17e26
remove debug code; register 'keys' in _TryUpdateKeyChord
carlos-zamora Aug 13, 2024
e658ffb
spell
carlos-zamora Aug 13, 2024
1ea1c51
spell the spell
carlos-zamora Aug 13, 2024
65defc0
exclude false positives in profiles
carlos-zamora Aug 21, 2024
ff22605
more false positive handling (and better logic)
carlos-zamora Aug 22, 2024
f201fee
appease the spell checker
carlos-zamora Aug 22, 2024
9b04a62
spell rite
carlos-zamora Aug 22, 2024
d7b6b54
feedback
carlos-zamora Aug 23, 2024
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
6 changes: 2 additions & 4 deletions src/cascadia/TerminalSettingsModel/ActionMap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
Json::Value ToJson() const;
Json::Value KeyBindingsToJson() const;
bool FixupsAppliedDuringLoad() const;
void LogSettingChanges(std::set<std::string_view>& changes, std::string_view& context) const;
void LogSettingChanges(std::set<std::string>& changes, const std::string& context) const;

// modification
bool RebindKeys(const Control::KeyChord& oldKeys, const Control::KeyChord& newKeys);
Expand Down Expand Up @@ -105,8 +105,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation

std::vector<Model::Command> _updateLocalSnippetCache(winrt::hstring currentWorkingDirectory);

void _logCommand(const Model::Command& cmd);

Windows::Foundation::Collections::IMap<hstring, Model::ActionAndArgs> _AvailableActionsCache{ nullptr };
Windows::Foundation::Collections::IMap<hstring, Model::Command> _NameMapCache{ nullptr };
Windows::Foundation::Collections::IMap<Control::KeyChord, Model::Command> _GlobalHotkeysCache{ nullptr };
Expand Down Expand Up @@ -141,7 +139,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation

til::shared_mutex<std::unordered_map<hstring, std::vector<Model::Command>>> _cwdLocalSnippetsCache{};

std::set<std::string_view> _changeLog;
std::set<std::string> _changeLog;

friend class SettingsModelUnitTests::KeyBindingsTests;
friend class SettingsModelUnitTests::DeserializationTests;
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
return keybindingsList;
}

void ActionMap::LogSettingChanges(std::set<std::string_view>& changes, std::string_view& context) const
void ActionMap::LogSettingChanges(std::set<std::string>& changes, const std::string& context) const
{
for (const auto& setting : _changeLog)
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
{
Expand Down
10 changes: 5 additions & 5 deletions src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ void AppearanceConfig::LayerJson(const Json::Value& json)
// to make the UI happy, set ColorSchemeName.
JsonUtils::GetValueForKey(json, ColorSchemeKey, _DarkColorSchemeName);
_LightColorSchemeName = _DarkColorSchemeName;
_logSettingSet(ColorSchemeKey);
_logSettingSet(std::string{ ColorSchemeKey });
}
else if (json["colorScheme"].isObject())
{
Expand Down Expand Up @@ -172,20 +172,20 @@ winrt::hstring AppearanceConfig::ExpandedBackgroundImagePath()
}
}

void AppearanceConfig::_logSettingSet(std::string_view setting)
void AppearanceConfig::_logSettingSet(const std::string& setting)
{
_changeLog.emplace(setting);
}

void AppearanceConfig::_logSettingIfSet(std::string_view setting, const bool isSet)
void AppearanceConfig::_logSettingIfSet(const std::string_view& setting, const bool isSet)
{
if (isSet)
{
_logSettingSet(setting);
_logSettingSet(std::string{ setting });
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
}
}

void AppearanceConfig::LogSettingChanges(std::set<std::string_view>& changes, std::string_view& context) const
void AppearanceConfig::LogSettingChanges(std::set<std::string>& changes, const std::string& context) const
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
{
for (const auto& setting : _changeLog)
{
Expand Down
8 changes: 4 additions & 4 deletions src/cascadia/TerminalSettingsModel/AppearanceConfig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
static winrt::com_ptr<AppearanceConfig> CopyAppearance(const AppearanceConfig* source, winrt::weak_ref<Profile> sourceProfile);
Json::Value ToJson() const;
void LayerJson(const Json::Value& json);
void LogSettingChanges(std::set<std::string_view>& changes, std::string_view& context) const;
void LogSettingChanges(std::set<std::string>& changes, const std::string& context) const;

Model::Profile SourceProfile();

Expand All @@ -53,9 +53,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation

private:
winrt::weak_ref<Profile> _sourceProfile;
std::set<std::string_view> _changeLog;
std::set<std::string> _changeLog;

void _logSettingSet(std::string_view setting);
void _logSettingIfSet(std::string_view setting, const bool isSet);
void _logSettingSet(const std::string& setting);
void _logSettingIfSet(const std::string_view& setting, const bool isSet);
};
}
7 changes: 3 additions & 4 deletions src/cascadia/TerminalSettingsModel/CascadiaSettings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
std::unordered_map<winrt::hstring, winrt::com_ptr<implementation::ColorScheme>> colorSchemes;
std::unordered_map<winrt::hstring, winrt::hstring> colorSchemeRemappings;
bool fixupsAppliedDuringLoad{ false };
std::set<std::string_view> themesChangeLog;
std::set<std::string> themesChangeLog;

void clear();
};
Expand Down Expand Up @@ -97,7 +97,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
void _executeGenerator(const IDynamicProfileGenerator& generator);

std::unordered_set<std::wstring_view> _ignoredNamespaces;
std::set<std::string_view> themesChangeLog;
std::set<std::string> themesChangeLog;
// See _getNonUserOriginProfiles().
size_t _userProfileCount = 0;
};
Expand Down Expand Up @@ -177,15 +177,14 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
void _validateThemeExists();

void _researchOnLoad();
std::set<std::string_view> _logSettingChanges() const;

// user settings
winrt::hstring _hash;
winrt::com_ptr<implementation::GlobalAppSettings> _globals = winrt::make_self<implementation::GlobalAppSettings>();
winrt::com_ptr<implementation::Profile> _baseLayerProfile = winrt::make_self<implementation::Profile>();
winrt::Windows::Foundation::Collections::IObservableVector<Model::Profile> _allProfiles = winrt::single_threaded_observable_vector<Model::Profile>();
winrt::Windows::Foundation::Collections::IObservableVector<Model::Profile> _activeProfiles = winrt::single_threaded_observable_vector<Model::Profile>();
std::set<std::string_view> _themesChangeLog{};
std::set<std::string> _themesChangeLog{};

// load errors
winrt::Windows::Foundation::Collections::IVector<Model::SettingsLoadWarnings> _warnings = winrt::single_threaded_vector<Model::SettingsLoadWarnings>();
Expand Down
47 changes: 25 additions & 22 deletions src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,7 @@ void SettingsLoader::_parse(const OriginTag origin, const winrt::hstring& source

if (origin != OriginTag::InBox)
{
static std::string_view themesContext{ "themes" };
static std::string themesContext{ "themes" };
theme->LogSettingChanges(settings.themesChangeLog, themesContext);
PankajBhojwani marked this conversation as resolved.
Show resolved Hide resolved
}
settings.globals->AddTheme(*theme);
Expand Down Expand Up @@ -1589,48 +1589,46 @@ void CascadiaSettings::_resolveNewTabMenuProfilesSet(const IVector<Model::NewTab
}
}

std::set<std::string_view> CascadiaSettings::_logSettingChanges() const
void CascadiaSettings::LogSettingChanges(bool isJsonLoad) const
{
#ifndef _DEBUG
// Only do this if we're actually being sampled
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
if (!TraceLoggingProviderEnabled(g_hSettingsModelProvider, 0, MICROSOFT_KEYWORD_MEASURES))
{
return;
}
#endif // !_DEBUG

// aggregate setting changes
std::set<std::string_view> changes;
static std::string_view globalContext{ "global" };
std::set<std::string> changes;
static std::string globalContext{ "global" };
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
_globals->LogSettingChanges(changes, globalContext);

static std::string_view actionContext{ "action" };
// Actions are not expected to change when loaded from the settings UI
static std::string actionContext{ "action" };
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
winrt::get_self<implementation::ActionMap>(_globals->ActionMap())->LogSettingChanges(changes, actionContext);

static std::string_view profileContext{ "profile" };
static std::string profileContext{ "profile" };
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
for (const auto& profile : _allProfiles)
{
winrt::get_self<Profile>(profile)->LogSettingChanges(changes, profileContext);
}

static std::string_view profileDefaultsContext{ "profileDefaults" };
static std::string profileDefaultsContext{ "profileDefaults" };
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
_baseLayerProfile->LogSettingChanges(changes, profileDefaultsContext);

// Themes are not expected to change when loaded from the settings UI
// DO NOT CALL Theme::LogSettingChanges!!
// We already collected the changes when we loaded the JSON
for (const auto& change : _themesChangeLog)
{
changes.insert(change);
}

return changes;
}

void CascadiaSettings::LogSettingChanges(bool isJsonLoad) const
{
// Only do this if we're actually being sampled
if (!TraceLoggingProviderEnabled(g_hSettingsModelProvider, 0, MICROSOFT_KEYWORD_MEASURES))
{
return;
}

const auto changes = _logSettingChanges();

// report changes
for (const auto& change : changes)
{
#ifndef _DEBUG
// A `isJsonLoad ? "JsonSettingsChanged" : "UISettingsChanged"`
// would be nice, but that apparently isn't allowed in the macro below.
// Also, there's guidance to not send too much data all in one event,
Expand All @@ -1639,7 +1637,7 @@ void CascadiaSettings::LogSettingChanges(bool isJsonLoad) const
{
TraceLoggingWrite(g_hSettingsModelProvider,
"JsonSettingsChanged",
Copy link
Member

Choose a reason for hiding this comment

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

we may get flagged with a "garrulous event". this is a lot of data. there might be a way to TraceLog an array??? i don't actually know.

my concern is that we'll get absolutely whacked with data for any even moderately complicated settings file

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a limit of 65535 bytes per event so that's my concern (though idk how valid it may be) in packing it all together into one.

Even then though, if we sent it over as an array, won't that exclude the stuff we actually want to learn? Like, if we received events with a payload of...

  • globals.initialRows, globals.wordDelimiters, globals.copyOnSelect
  • globals.wordDelimiters, globals.copyOnSelect

We would just see the data as 2 hits with a payload of an array each whereas we want to track "how many hits each setting got (i.e. globals.initialRows=1, globals.wordDelimiters=2, globals.copyOnSelect=2). Right?

TraceLoggingDescription("Event emitted when settings change"),
TraceLoggingDescription("Event emitted when settings.json change"),
TraceLoggingValue(change.data()),
TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES),
TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage));
Expand All @@ -1648,10 +1646,15 @@ void CascadiaSettings::LogSettingChanges(bool isJsonLoad) const
{
TraceLoggingWrite(g_hSettingsModelProvider,
"UISettingsChanged",
TraceLoggingDescription("Event emitted when settings change"),
TraceLoggingDescription("Event emitted when settings change via the UI"),
TraceLoggingValue(change.data()),
TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES),
TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage));
}
#else
OutputDebugStringA(isJsonLoad ? "JsonSettingsChanged\n" : "UISettingsChanged\n");
OutputDebugStringA(change.data());
OutputDebugStringA("\n");
#endif // !_DEBUG
}
}
12 changes: 6 additions & 6 deletions src/cascadia/TerminalSettingsModel/Command.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,29 +742,29 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
return winrt::single_threaded_vector<Model::Command>(std::move(result));
}

void Command::LogSettingChanges(std::set<std::string_view>& changes)
void Command::LogSettingChanges(std::set<std::string>& changes)
PankajBhojwani marked this conversation as resolved.
Show resolved Hide resolved
{
if (_IterateOn != ExpandCommandType::None)
{
switch (_IterateOn)
{
case ExpandCommandType::Profiles:
changes.insert(fmt::format(FMT_COMPILE("{}.{}"), IterateOnKey, "profiles"));
changes.insert(fmt::format(FMT_COMPILE("{}.{}"), std::string{ IterateOnKey }, "profiles"));
break;
case ExpandCommandType::ColorSchemes:
changes.insert(fmt::format(FMT_COMPILE("{}.{}"), IterateOnKey, "schemes"));
changes.insert(fmt::format(FMT_COMPILE("{}.{}"), std::string{ IterateOnKey }, "schemes"));
break;
}
}

if (!_Description.empty())
{
changes.insert(fmt::format(FMT_COMPILE("{}"), DescriptionKey));
changes.insert(fmt::format(FMT_COMPILE("{}"), std::string{ DescriptionKey }));
}

if (IsNestedCommand())
{
changes.insert(fmt::format(FMT_COMPILE("{}"), CommandsKey));
changes.insert(fmt::format(FMT_COMPILE("{}"), std::string{ CommandsKey }));
}
else
{
Expand All @@ -782,7 +782,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
// - "command": { "action": "copy", "singleLine": true } --> "copy.singleLine"
// - "command": { "action": "copy", "singleLine": true, "dismissSelection": true } --> "copy.singleLine", "copy.dismissSelection"

std::string_view shortcutActionName{ json[JsonKey("action")].asString() };
const std::string shortcutActionName{ json[JsonKey("action")].asString() };

auto members = json.getMemberNames();
members.erase(std::remove_if(members.begin(), members.end(), [](const auto& member) { return member == "action"; }), members.end());
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalSettingsModel/Command.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
const Json::Value& json,
const OriginTag origin);
Json::Value ToJson() const;
void LogSettingChanges(std::set<std::string_view>& changes);
void LogSettingChanges(std::set<std::string>& changes);

bool HasNestedCommands() const;
bool IsNestedCommand() const noexcept;
Expand Down
6 changes: 3 additions & 3 deletions src/cascadia/TerminalSettingsModel/FontConfig.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ winrt::Microsoft::Terminal::Settings::Model::Profile FontConfig::SourceProfile()
return _sourceProfile.get();
}

void FontConfig::_logSettingSet(std::string_view setting)
void FontConfig::_logSettingSet(const std::string& setting)
{
if (setting == "axes" && _FontAxes.has_value())
{
Expand All @@ -132,15 +132,15 @@ void FontConfig::_logSettingSet(std::string_view setting)
}
}

void FontConfig::_logSettingIfSet(std::string_view setting, const bool isSet)
void FontConfig::_logSettingIfSet(const std::string& setting, const bool isSet)
{
if (isSet)
{
_logSettingSet(setting);
}
}

void FontConfig::LogSettingChanges(std::set<std::string_view>& changes, std::string_view& context) const
void FontConfig::LogSettingChanges(std::set<std::string>& changes, const std::string& context) const
{
for (const auto& setting : _changeLog)
{
Expand Down
8 changes: 4 additions & 4 deletions src/cascadia/TerminalSettingsModel/FontConfig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
static winrt::com_ptr<FontConfig> CopyFontInfo(const FontConfig* source, winrt::weak_ref<Profile> sourceProfile);
Json::Value ToJson() const;
void LayerJson(const Json::Value& json);
void LogSettingChanges(std::set<std::string_view>& changes, std::string_view& context) const;
void LogSettingChanges(std::set<std::string>& changes, const std::string& context) const;

Model::Profile SourceProfile();

Expand All @@ -46,9 +46,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation

private:
winrt::weak_ref<Profile> _sourceProfile;
std::set<std::string_view> _changeLog;
std::set<std::string> _changeLog;

void _logSettingSet(std::string_view setting);
void _logSettingIfSet(std::string_view setting, const bool isSet);
void _logSettingSet(const std::string& setting);
void _logSettingIfSet(const std::string& setting, const bool isSet);
};
}
18 changes: 9 additions & 9 deletions src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ void GlobalAppSettings::LayerJson(const Json::Value& json, const OriginTag origi
JsonUtils::GetValueForKey(json, LegacyReloadEnvironmentVariablesKey, _legacyReloadEnvironmentVariables);
if (json[LegacyReloadEnvironmentVariablesKey.data()])
{
_logSettingSet(LegacyReloadEnvironmentVariablesKey);
_logSettingSet(std::string{ LegacyReloadEnvironmentVariablesKey });
}
}

Expand Down Expand Up @@ -325,7 +325,7 @@ bool GlobalAppSettings::ShouldUsePersistedLayout() const
return FirstWindowPreference() == FirstWindowPreference::PersistedWindowLayout && !IsolatedMode();
}

void GlobalAppSettings::_logSettingSet(std::string_view setting)
void GlobalAppSettings::_logSettingSet(const std::string& setting)
{
if (setting == "theme")
{
Expand All @@ -339,8 +339,8 @@ void GlobalAppSettings::_logSettingSet(std::string_view setting)
}
else
{
_changeLog.insert(std::string_view{ fmt::format(FMT_COMPILE("{}.{}"), setting, "dark") });
_changeLog.insert(std::string_view{ fmt::format(FMT_COMPILE("{}.{}"), setting, "light") });
_changeLog.emplace(fmt::format(FMT_COMPILE("{}.{}"), setting, "dark"));
_changeLog.emplace(fmt::format(FMT_COMPILE("{}.{}"), setting, "light"));
}
}
}
Expand Down Expand Up @@ -375,25 +375,25 @@ void GlobalAppSettings::_logSettingSet(std::string_view setting)
// ignore invalid
continue;
}
_changeLog.insert(std::string_view{ fmt::format(FMT_COMPILE("{}.{}"), setting, entryType) });
_changeLog.emplace(fmt::format(FMT_COMPILE("{}.{}"), setting, entryType));
}
}
}
else
{
_changeLog.insert(setting);
_changeLog.emplace(setting);
}
}

void GlobalAppSettings::_logSettingIfSet(std::string_view setting, const bool isSet)
void GlobalAppSettings::_logSettingIfSet(const std::string& setting, const bool isSet)
{
if (isSet)
{
_logSettingSet(setting);
_logSettingSet(std::string{ setting });
}
}

void GlobalAppSettings::LogSettingChanges(std::set<std::string_view>& changes, std::string_view& context) const
void GlobalAppSettings::LogSettingChanges(std::set<std::string>& changes, const std::string& context) const
{
for (const auto& setting : _changeLog)
{
Expand Down
Loading
Loading