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 and log changes to settings #17678

Merged
merged 16 commits into from
Aug 23, 2024
Merged
2 changes: 2 additions & 0 deletions .github/actions/spelling/allow/allow.txt
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ gje
godbolt
hyperlinking
hyperlinks
Kbds
kje
libfuzzer
liga
Expand All @@ -43,6 +44,7 @@ mkmk
mnt
mru
nje
NTMTo
notwrapped
ogonek
overlined
Expand Down
4 changes: 4 additions & 0 deletions src/cascadia/TerminalApp/AppLogic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,10 @@ namespace winrt::TerminalApp::implementation
return;
}
}
else
{
_settings.LogSettingChanges(true);
}

if (initialLoad)
{
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalSettingsEditor/MainPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,7 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation

void MainPage::SaveButton_Click(const IInspectable& /*sender*/, const RoutedEventArgs& /*args*/)
{
_settingsClone.LogSettingChanges(false);
_settingsClone.WriteSettingsToDisk();
}

Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalSettingsModel/ActionMap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
const auto action = cmd.ActionAndArgs().Action();
const auto id = action == ShortcutAction::Invalid ? hstring{} : cmd.ID();
_KeyMap.insert_or_assign(keys, id);
_changeLog.emplace(KeysKey);
}

// Method Description:
Expand Down
3 changes: 3 additions & 0 deletions src/cascadia/TerminalSettingsModel/ActionMap.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +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>& changes, const std::string_view& context) const;

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

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

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

friend class SettingsModelUnitTests::KeyBindingsTests;
friend class SettingsModelUnitTests::DeserializationTests;
friend class SettingsModelUnitTests::TerminalSettingsTests;
Expand Down
34 changes: 33 additions & 1 deletion src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
// Now check if this is a command block
if (jsonBlock.isMember(JsonKey(CommandsKey)) || jsonBlock.isMember(JsonKey(ActionKey)))
{
AddAction(*Command::FromJson(jsonBlock, warnings, origin), keys);
auto command = Command::FromJson(jsonBlock, warnings, origin);
command->LogSettingChanges(_changeLog);
PankajBhojwani marked this conversation as resolved.
Show resolved Hide resolved
AddAction(*command, keys);

if (jsonBlock.isMember(JsonKey(KeysKey)))
{
Expand All @@ -105,6 +107,28 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation

// any existing keybinding with the same keychord in this layer will get overwritten
_KeyMap.insert_or_assign(keys, idJson);

if (!_changeLog.contains(KeysKey.data()))
Copy link
Member

Choose a reason for hiding this comment

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

Elsewhere you wrote _changeLog.emplace(KeysKey);. Does this not support the contains call without the .data() call?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope :/. Even with the transparent comparators

{
// Log "keys" field, but only if it's one that isn't in userDefaults.json
static constexpr std::array<std::pair<std::wstring_view, std::string_view>, 3> userDefaultKbds{ { { L"Terminal.CopyToClipboard", "ctrl+c" },
Fixed Show fixed Hide fixed
{ L"Terminal.PasteFromClipboard", "ctrl+v" },
{ L"Terminal.DuplicatePaneAuto", "alt+shift+d" } } };
bool isUserDefaultKbd = false;
for (const auto& [id, kbd] : userDefaultKbds)
Fixed Show fixed Hide fixed
{
const auto keyJson{ jsonBlock.find(&*KeysKey.cbegin(), (&*KeysKey.cbegin()) + KeysKey.size()) };
lhecker marked this conversation as resolved.
Show resolved Hide resolved
if (idJson == id && keyJson->asString() == kbd)
{
isUserDefaultKbd = true;
break;
}
}
if (!isUserDefaultKbd)
{
_changeLog.emplace(KeysKey);
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
}

Expand Down Expand Up @@ -156,4 +180,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation

return keybindingsList;
}

void ActionMap::LogSettingChanges(std::set<std::string>& changes, const std::string_view& context) const
{
for (const auto& setting : _changeLog)
carlos-zamora marked this conversation as resolved.
Show resolved Hide resolved
{
changes.emplace(fmt::format(FMT_COMPILE("{}.{}"), context, setting));
}
}
}
38 changes: 37 additions & 1 deletion src/cascadia/TerminalSettingsModel/AppearanceConfig.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -90,27 +90,42 @@ Json::Value AppearanceConfig::ToJson() const
void AppearanceConfig::LayerJson(const Json::Value& json)
{
JsonUtils::GetValueForKey(json, ForegroundKey, _Foreground);
_logSettingIfSet(ForegroundKey, _Foreground.has_value());

JsonUtils::GetValueForKey(json, BackgroundKey, _Background);
_logSettingIfSet(BackgroundKey, _Background.has_value());

JsonUtils::GetValueForKey(json, SelectionBackgroundKey, _SelectionBackground);
_logSettingIfSet(SelectionBackgroundKey, _SelectionBackground.has_value());

JsonUtils::GetValueForKey(json, CursorColorKey, _CursorColor);
_logSettingIfSet(CursorColorKey, _CursorColor.has_value());

JsonUtils::GetValueForKey(json, LegacyAcrylicTransparencyKey, _Opacity);
JsonUtils::GetValueForKey(json, OpacityKey, _Opacity, JsonUtils::OptionalConverter<float, IntAsFloatPercentConversionTrait>{});
_logSettingIfSet(OpacityKey, _Opacity.has_value());

if (json["colorScheme"].isString())
{
// to make the UI happy, set ColorSchemeName.
JsonUtils::GetValueForKey(json, ColorSchemeKey, _DarkColorSchemeName);
_LightColorSchemeName = _DarkColorSchemeName;
_logSettingSet(ColorSchemeKey);
}
else if (json["colorScheme"].isObject())
{
// to make the UI happy, set ColorSchemeName to whatever the dark value is.
JsonUtils::GetValueForKey(json["colorScheme"], "dark", _DarkColorSchemeName);
JsonUtils::GetValueForKey(json["colorScheme"], "light", _LightColorSchemeName);

_logSettingSet("colorScheme.dark");
_logSettingSet("colorScheme.light");
}

#define APPEARANCE_SETTINGS_LAYER_JSON(type, name, jsonKey, ...) \
JsonUtils::GetValueForKey(json, jsonKey, _##name);
JsonUtils::GetValueForKey(json, jsonKey, _##name); \
_logSettingIfSet(jsonKey, _##name.has_value());

MTSM_APPEARANCE_SETTINGS(APPEARANCE_SETTINGS_LAYER_JSON)
#undef APPEARANCE_SETTINGS_LAYER_JSON
}
Expand Down Expand Up @@ -156,3 +171,24 @@ winrt::hstring AppearanceConfig::ExpandedBackgroundImagePath()
return winrt::hstring{ wil::ExpandEnvironmentStringsW<std::wstring>(path.c_str()) };
}
}

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

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

void AppearanceConfig::LogSettingChanges(std::set<std::string>& changes, const std::string_view& context) const
{
for (const auto& setting : _changeLog)
{
changes.emplace(fmt::format(FMT_COMPILE("{}.{}"), context, setting));
}
}
5 changes: 5 additions & 0 deletions src/cascadia/TerminalSettingsModel/AppearanceConfig.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +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>& changes, const std::string_view& context) const;

Model::Profile SourceProfile();

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

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

void _logSettingSet(const std::string_view& setting);
void _logSettingIfSet(const std::string_view& setting, const bool isSet);
};
}
5 changes: 5 additions & 0 deletions src/cascadia/TerminalSettingsModel/CascadiaSettings.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +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> themesChangeLog;

void clear();
};
Expand Down Expand Up @@ -96,6 +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> themesChangeLog;
// See _getNonUserOriginProfiles().
size_t _userProfileCount = 0;
};
Expand Down Expand Up @@ -150,6 +152,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation

void ExpandCommands();

void LogSettingChanges(bool isJsonLoad) const;

private:
static const std::filesystem::path& _settingsPath();
static const std::filesystem::path& _releaseSettingsPath();
Expand Down Expand Up @@ -180,6 +184,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
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> _themesChangeLog{};

// load errors
winrt::Windows::Foundation::Collections::IVector<Model::SettingsLoadWarnings> _warnings = winrt::single_threaded_vector<Model::SettingsLoadWarnings>();
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalSettingsModel/CascadiaSettings.idl
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ namespace Microsoft.Terminal.Settings.Model

CascadiaSettings Copy();
void WriteSettingsToDisk();
void LogSettingChanges(Boolean isJsonLoad);

String Hash { get; };

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ void ParsedSettings::clear()
profilesByGuid.clear();
colorSchemes.clear();
fixupsAppliedDuringLoad = false;
themesChangeLog.clear();
}

// This is a convenience method used by the CascadiaSettings constructor.
Expand Down Expand Up @@ -642,6 +643,12 @@ void SettingsLoader::_parse(const OriginTag origin, const winrt::hstring& source
// versions of these themes overriding the built-in ones.
continue;
}

if (origin != OriginTag::InBox)
{
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 @@ -1206,6 +1213,7 @@ CascadiaSettings::CascadiaSettings(SettingsLoader&& loader) :
_allProfiles = winrt::single_threaded_observable_vector(std::move(allProfiles));
_activeProfiles = winrt::single_threaded_observable_vector(std::move(activeProfiles));
_warnings = winrt::single_threaded_vector(std::move(warnings));
_themesChangeLog = std::move(loader.userSettings.themesChangeLog);

_resolveDefaultProfile();
_resolveNewTabMenuProfiles();
Expand Down Expand Up @@ -1580,3 +1588,73 @@ void CascadiaSettings::_resolveNewTabMenuProfilesSet(const IVector<Model::NewTab
}
}
}

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> changes;
static constexpr std::string_view globalContext{ "global" };
_globals->LogSettingChanges(changes, globalContext);

// Actions are not expected to change when loaded from the settings UI
static constexpr std::string_view actionContext{ "action" };
winrt::get_self<implementation::ActionMap>(_globals->ActionMap())->LogSettingChanges(changes, actionContext);

static constexpr std::string_view profileContext{ "profile" };
for (const auto& profile : _allProfiles)
{
winrt::get_self<Profile>(profile)->LogSettingChanges(changes, profileContext);
}

static constexpr std::string_view profileDefaultsContext{ "profileDefaults" };
_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);
}

// 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,
// so we'll be sending a ton of events here.
if (isJsonLoad)
{
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.json change"),
TraceLoggingValue(change.data()),
TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES),
TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage));
}
else
{
TraceLoggingWrite(g_hSettingsModelProvider,
"UISettingsChanged",
TraceLoggingDescription("Event emitted when settings change via the UI"),
TraceLoggingValue(change.data()),
TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES),
TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage));
}
#else
OutputDebugStringA(isJsonLoad ? "JsonSettingsChanged - " : "UISettingsChanged - ");
Copy link
Member

Choose a reason for hiding this comment

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

very clever.

OutputDebugStringA(change.data());
OutputDebugStringA("\n");
#endif // !_DEBUG
}
}
Loading
Loading