From 4f2c03d2b994c81a9c5bfedd590e14872e50dfd9 Mon Sep 17 00:00:00 2001 From: eric langlois Date: Mon, 18 Apr 2022 19:28:25 -0700 Subject: [PATCH] Push Notifications Telemetry Improvements (#2401) * Update push telemetry to match toast telemetry * UnregisterAll needs to be tracked * Don;t log the same event twice Co-authored-by: Eric Langlois --- .../PushNotificationChannel.cpp | 9 +- .../PushNotificationManager.cpp | 31 +++++-- .../PushNotificationTelemetry.h | 88 +++++++++---------- 3 files changed, 73 insertions(+), 55 deletions(-) diff --git a/dev/PushNotifications/PushNotificationChannel.cpp b/dev/PushNotifications/PushNotificationChannel.cpp index 17e2b8d6e1..8446384afa 100644 --- a/dev/PushNotifications/PushNotificationChannel.cpp +++ b/dev/PushNotifications/PushNotificationChannel.cpp @@ -39,17 +39,22 @@ namespace winrt::Microsoft::Windows::PushNotifications::implementation void PushNotificationChannel::Close() { + HRESULT hr{ S_OK }; + + auto logTelemetry{ wil::scope_exit([&]() { + PushNotificationTelemetry::LogCloseChannel(hr); + }) }; + try { THROW_IF_FAILED(PushNotifications_CloseChannel(m_channelInfo.appId.c_str(), m_channelInfo.channelId.c_str())); - PushNotificationTelemetry::ChannelClosedByApi(S_OK); } catch (...) { auto channelCloseException { hresult_error(to_hresult()) }; - PushNotificationTelemetry::ChannelClosedByApi(channelCloseException.code()); + hr = channelCloseException.code(); if (channelCloseException.code() != HRESULT_FROM_WIN32(ERROR_NOT_FOUND)) { diff --git a/dev/PushNotifications/PushNotificationManager.cpp b/dev/PushNotifications/PushNotificationManager.cpp index e688a9397b..c6c49fac54 100644 --- a/dev/PushNotifications/PushNotificationManager.cpp +++ b/dev/PushNotifications/PushNotificationManager.cpp @@ -209,6 +209,12 @@ namespace winrt::Microsoft::Windows::PushNotifications::implementation PushNotificationChannelStatus::CompletedFailure); } + HRESULT hr{ S_OK }; + + auto logTelemetry{ wil::scope_exit([&]() { + PushNotificationTelemetry::LogCreateChannelAsync(hr, remoteId); + }) }; + auto strong = get_strong(); try @@ -295,7 +301,6 @@ namespace winrt::Microsoft::Windows::PushNotifications::implementation m_channel = channel; } - PushNotificationTelemetry::ChannelRequestedByApi(S_OK, remoteId); co_return winrt::make( channel, S_OK, @@ -315,7 +320,7 @@ namespace winrt::Microsoft::Windows::PushNotifications::implementation } else { - PushNotificationTelemetry::ChannelRequestedByApi(channelRequestException.code(), remoteId); + hr = channelRequestException.code(); co_return winrt::make( nullptr, @@ -328,7 +333,7 @@ namespace winrt::Microsoft::Windows::PushNotifications::implementation } catch (...) { - PushNotificationTelemetry::ChannelRequestedByApi(wil::ResultFromCaughtException(), remoteId); + hr = wil::ResultFromCaughtException(); throw; } } @@ -365,6 +370,11 @@ namespace winrt::Microsoft::Windows::PushNotifications::implementation } winrt::hresult hr{ S_OK }; + + auto logTelemetry{ wil::scope_exit([&]() { + PushNotificationTelemetry::LogRegister(hr); + }) }; + try { { @@ -592,7 +602,6 @@ namespace winrt::Microsoft::Windows::PushNotifications::implementation hr = wil::ResultFromCaughtException(); } - PushNotificationTelemetry::ActivatorRegisteredByApi(hr); THROW_IF_FAILED(hr); } @@ -604,6 +613,11 @@ namespace winrt::Microsoft::Windows::PushNotifications::implementation } winrt::hresult hr{ S_OK }; + + auto logTelemetry{ wil::scope_exit([&]() { + PushNotificationTelemetry::LogUnregister(hr); + }) }; + try { { @@ -667,7 +681,6 @@ namespace winrt::Microsoft::Windows::PushNotifications::implementation hr = wil::ResultFromCaughtException(); } - PushNotificationTelemetry::ActivatorUnregisteredByApi(hr); THROW_IF_FAILED(hr); } @@ -678,6 +691,12 @@ namespace winrt::Microsoft::Windows::PushNotifications::implementation return; } + hresult hr = S_OK; + + auto logTelemetry{ wil::scope_exit([&]() { + PushNotificationTelemetry::LogUnregisterAll(hr); + }) }; + bool comActivatorRegistration{ false }; bool singletonForegroundRegistration{ false }; { @@ -691,7 +710,6 @@ namespace winrt::Microsoft::Windows::PushNotifications::implementation Unregister(); } - hresult hr = S_OK; try { { @@ -748,7 +766,6 @@ namespace winrt::Microsoft::Windows::PushNotifications::implementation hr = wil::ResultFromCaughtException(); } - PushNotificationTelemetry::ActivatorUnregisteredByApi(hr); THROW_IF_FAILED(hr); } diff --git a/dev/PushNotifications/PushNotificationTelemetry.h b/dev/PushNotifications/PushNotificationTelemetry.h index e58e6ea1b1..595513d92e 100644 --- a/dev/PushNotifications/PushNotificationTelemetry.h +++ b/dev/PushNotifications/PushNotificationTelemetry.h @@ -16,69 +16,84 @@ class PushNotificationTelemetry : public wil::TraceLoggingProvider IMPLEMENT_TELEMETRY_CLASS(PushNotificationTelemetry, PushNotificationTelemetryProvider); public: - DEFINE_EVENT_METHOD(ChannelRequestedByApi)( + DEFINE_EVENT_METHOD(LogCreateChannelAsync)( winrt::hresult hr, const winrt::guid& remoteId) noexcept try { if (c_maxEventLimit >= UpdateLogEventCount()) { TraceLoggingClassWriteMeasure( - "ChannelRequestedByApi", + "CreateChannelAsync", TelemetryPrivacyDataTag(PDT_ProductAndServicePerformance), _GENERIC_PARTB_FIELDS_ENABLED, TraceLoggingHexUInt32(hr, "OperationResult"), TraceLoggingGuid(remoteId, "RemoteId"), TraceLoggingBool(IsPackagedApp(), "IsAppPackaged"), - TraceLoggingWideString(GetAppName(), "AppName")); + TraceLoggingWideString(GetAppName().c_str(), "AppName")); } } CATCH_LOG() - DEFINE_EVENT_METHOD(ChannelClosedByApi)( + DEFINE_EVENT_METHOD(LogCloseChannel)( winrt::hresult hr) noexcept try { if (c_maxEventLimit >= UpdateLogEventCount()) { TraceLoggingClassWriteMeasure( - "ChannelClosedByApi", + "CloseChannel", TelemetryPrivacyDataTag(PDT_ProductAndServicePerformance), _GENERIC_PARTB_FIELDS_ENABLED, TraceLoggingHexUInt32(hr, "OperationResult"), TraceLoggingBool(IsPackagedApp(), "IsAppPackaged"), - TraceLoggingWideString(GetAppName(), "AppName")); + TraceLoggingWideString(GetAppName().c_str(), "AppName")); } } CATCH_LOG() - DEFINE_EVENT_METHOD(ActivatorRegisteredByApi)( + DEFINE_EVENT_METHOD(LogRegister)( winrt::hresult hr) noexcept try { if (c_maxEventLimit >= UpdateLogEventCount()) { TraceLoggingClassWriteMeasure( - "ActivatorRegisteredByApi", - TelemetryPrivacyDataTag(PDT_ProductAndServicePerformance), + "Register", + TelemetryPrivacyDataTag(PDT_ProductAndServicePerformance), _GENERIC_PARTB_FIELDS_ENABLED, TraceLoggingHexUInt32(hr, "OperationResult"), TraceLoggingBool(IsPackagedApp(), "IsAppPackaged"), - TraceLoggingWideString(GetAppName(), "AppName")); + TraceLoggingWideString(GetAppName().c_str(), "AppName")); } } CATCH_LOG() + DEFINE_EVENT_METHOD(LogUnregister)( + winrt::hresult hr) noexcept try + { + if (c_maxEventLimit >= UpdateLogEventCount()) + { + TraceLoggingClassWriteMeasure( + "Unregister", + TelemetryPrivacyDataTag(PDT_ProductAndServicePerformance), + _GENERIC_PARTB_FIELDS_ENABLED, + TraceLoggingHexUInt32(hr, "OperationResult"), + TraceLoggingBool(IsPackagedApp(), "IsAppPackaged"), + TraceLoggingWideString(GetAppName().c_str(), "AppName")); + } + } + CATCH_LOG() - DEFINE_EVENT_METHOD(ActivatorUnregisteredByApi)( + DEFINE_EVENT_METHOD(LogUnregisterAll)( winrt::hresult hr) noexcept try { if (c_maxEventLimit >= UpdateLogEventCount()) { TraceLoggingClassWriteMeasure( - "ActivatorUnregisteredByApi", + "UnregisterAll", TelemetryPrivacyDataTag(PDT_ProductAndServicePerformance), _GENERIC_PARTB_FIELDS_ENABLED, TraceLoggingHexUInt32(hr, "OperationResult"), TraceLoggingBool(IsPackagedApp(), "IsAppPackaged"), - TraceLoggingWideString(GetAppName(), "AppName")); + TraceLoggingWideString(GetAppName().c_str(), "AppName")); } } CATCH_LOG() @@ -91,7 +106,7 @@ class PushNotificationTelemetry : public wil::TraceLoggingProvider static constexpr ULONGLONG c_logPeriod = 1000; // One second static constexpr UINT c_maxEventLimit = 10; - UINT UpdateLogEventCount() noexcept + UINT UpdateLogEventCount() { ULONGLONG currentTick = GetTickCount64(); @@ -109,61 +124,42 @@ class PushNotificationTelemetry : public wil::TraceLoggingProvider return m_eventCount; } - inline bool IsPackagedApp() + inline bool IsPackagedApp() const { - static const bool isPackagedApp = AppModel::Identity::IsPackagedProcess(); + static const bool isPackagedApp{ AppModel::Identity::IsPackagedProcess() }; return isPackagedApp; } - inline const wchar_t* GetAppName() + inline const std::wstring& GetAppName() const { - static const std::wstring appName = IsPackagedApp() ? GetAppNamePackaged() : GetAppNameUnpackaged(); + static const std::wstring appName{ IsPackagedApp() ? GetAppNamePackaged() : GetAppNameUnpackaged() }; - return appName.c_str(); + return appName; } - std::wstring GetAppNamePackaged() noexcept + std::wstring GetAppNamePackaged() const { wchar_t appUserModelId[APPLICATION_USER_MODEL_ID_MAX_LENGTH]{}; - UINT32 appUserModelIdSize = ARRAYSIZE(appUserModelId); - auto result = GetCurrentApplicationUserModelId(&appUserModelIdSize, appUserModelId); - if (result != ERROR_SUCCESS) - { - wcscpy_s(appUserModelId, L"AppUserModelId not found"); - LOG_WIN32(result); - } + UINT32 appUserModelIdSize{ ARRAYSIZE(appUserModelId) }; + THROW_IF_WIN32_ERROR(GetCurrentApplicationUserModelId(&appUserModelIdSize, appUserModelId)); return appUserModelId; } - PCWSTR CensorFilePath(PCWSTR path) noexcept + std::wstring CensorFilePath(const std::wstring& filepath) const { - if (path) - { - path = !PathIsFileSpecW(path) ? PathFindFileNameW(path) : path; - } - - return path; + return { !PathIsFileSpecW(filepath.c_str()) ? PathFindFileNameW(filepath.c_str()) : filepath }; } - std::wstring GetAppNameUnpackaged() + std::wstring GetAppNameUnpackaged() const { std::wstring appName; wil::unique_cotaskmem_string processName; - auto result = wil::GetModuleFileNameExW(GetCurrentProcess(), nullptr, processName); - if (result == ERROR_SUCCESS) - { - appName = CensorFilePath(processName.get()); - } - else - { - appName = L"ModuleFileName not found"; - LOG_WIN32(result); - } + THROW_IF_FAILED(wil::GetModuleFileNameExW(GetCurrentProcess(), nullptr, processName)); - return appName; + return CensorFilePath(processName.get()); } };