Telemetry Update for Unpackaged Apps by loneursid · Pull Request #1385 · microsoft/WindowsAppSDK

loneursid · 2021-09-08T18:05:49Z

Log the processName as an alternative to the package name as the appId for unpackaged apps.

loneursid · 2021-09-08T19:06:17Z

-    wchar_t m_appUserModelId[APPLICATION_USER_MODEL_ID_MAX_LENGTH] = {};
+    inline bool IsPackagedApp()
+    {
+        static bool isPackagedApp = AppModel::Identity::IsPackagedProcess();


Is this a valid approach in COM /Winrt?

It's valid and thread safe C++ code but I'm concerned that it may not play well with Com/WinRT lifetime management.

loneursid · 2021-09-09T14:19:32Z

/azp run

azure-pipelines · 2021-09-09T14:19:38Z

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

sharath2727 · 2021-09-10T14:07:02Z

-                                S_OK,
-                                AppModel::Identity::IsPackagedProcess(),
-                                remoteId);
+                            PushNotificationTelemetry::ChannelRequestedByApi(S_OK, remoteId);


when going by the legacy approach - remoteId doesn't make sense. Can you distinguish between legacy and newer approach?

yet, CreateChannelAsync was called with a remoteId (even if it'a a null one) and this is really all this is logging.

Added a flag for the legacy case, are there other options we want to log beyond "legacy"?

sharath2727 · 2021-09-10T14:20:52Z

+        auto result = wil::GetModuleFileNameExW(GetCurrentProcess(), nullptr, processName);
+        if (result == ERROR_SUCCESS)
+        {
+            appId = CensorFilePath(processName.get());


why are we storing the processname as appId? I think we need to call into the LRP and access the storage and get the appId right?

It is just a field name. The GUID doesn't mean anything if not tied to a process name anyway.

appId is not the best name choice; I'll replace it with appName. I was looking for an abstraction that could hold PFN for packaged app and the process name for unpackaged. AppId seemed like a good idea at the time but it's already way to overloaded.

Renamed to appName

danielayala94 · 2021-09-14T18:49:06Z

@@ -31,8 +32,9 @@ class PushNotificationTelemetry : public wil::TraceLoggingProvider
                _GENERIC_PARTB_FIELDS_ENABLED,
                TraceLoggingHexUInt32(hr, "OperationResult"),
                TraceLoggingWideString(to_hstring(remoteId).data(), "RemoteId"),


Nit: Is TraceLoggingGuid supported in WAS?

DrusTheAxe

LGTM modulo minor comments

DrusTheAxe · 2021-09-14T19:04:54Z


    winrt::IAsyncOperationWithProgress<winrt::Microsoft::Windows::PushNotifications::PushNotificationCreateChannelResult, winrt::Microsoft::Windows::PushNotifications::PushNotificationCreateChannelStatus> PushNotificationManager::CreateChannelAsync(const winrt::guid &remoteId)
    {
+        bool usingLegacyImpl = false;


NIT: bool usingLegacyImpl{};

ES.23: Prefer the {}-initializer syntax

same comment applies throughout

DrusTheAxe · 2021-09-14T19:05:27Z


    winrt::IAsyncOperationWithProgress<winrt::Microsoft::Windows::PushNotifications::PushNotificationCreateChannelResult, winrt::Microsoft::Windows::PushNotifications::PushNotificationCreateChannelStatus> PushNotificationManager::CreateChannelAsync(const winrt::guid &remoteId)
    {
+        bool usingLegacyImpl = false;


NIT: usingLegacyImplementation

Avoid abbreviations

DrusTheAxe · 2021-09-14T19:07:42Z

                TraceLoggingWideString(to_hstring(remoteId).data(), "RemoteId"),
-                TraceLoggingWideString(GetAppUserModelId(), "AppUserModelId"));
+                TraceLoggingBool(usingLegacyImpl, "UsingLegacyImpl"),
+                TraceLoggingBool(IsPackagedApp(), "IsAppPackaged"),


Isn't 'App' redundant here?

SUGGESTION: IsPackaged

I kept the IsAppPackaged, to match the corresponding GetAppName.

DrusTheAxe · 2021-09-14T19:09:35Z

+    std::wstring GetAppNamePackaged() noexcept
    {
-        if (m_appUserModelId[0] == '\0')
+        wchar_t appUserModelId[APPLICATION_USER_MODEL_ID_MAX_LENGTH] = {};


NIT: wchar_t app[BLAH]{};

DrusTheAxe · 2021-09-14T19:11:00Z

+        return appUserModelId;
+    }
+
+    PCWSTR CensorFilePath(_In_opt_ PCWSTR path) noexcept


NIT: Remove _In_opt_

InOpt is redundant with PCWSTR

* Original skeleton * Corrected mismatches against PowerNotifications change * Correcting few nits * Added constants usage, std::call_once and improved abstraction * Added StartupTask exe * Fixed some bugs * Mostly renaming and cleaning * Removed timer externals and let winmain to own Platform lifetime * Correction -> factory should own the platform * Addressed lifetime crashes and other issues * Improved timer/event management * Addressed feedback * Fixed compilation issues in other flavor+archs and added minor comments * Added a unit test (more coming!) * Test enhancements * Addressed comments * Moved timer/event logic to a platform component * Install Frameworkudk for LRP (#1271) * Introducing unpackaged foreground activation for PushNotifications (#1336) * Cleaned original code * Added IWpnNotificationSink COM interface to LRP MSIX * Just stuff for testing. Needs to be undone * Still working on it * Com proxy objects not being stored correctly * Receiving Data from LRP and changed sink container * Foreground activation working * Fix first round of nits * Still working on adding nits * Fix following FI of main * Addressing nits * Use com_array instead of byte* * Update dev/PushNotifications/PushNotificationDummyDeferral.h Co-authored-by: Daniel Ayala <14967941+danielayala94@users.noreply.github.com> Co-authored-by: Daniel Ayala <joayalag@microsoft.com> Co-authored-by: Eric Langlois <erlangl@microsoft.com> Co-authored-by: Daniel Ayala <14967941+danielayala94@users.noreply.github.com> * Not everything should be renamed Runtime * Ensuring the LRP msix gets built after the framework (#1356) Co-authored-by: Eric Langlois <erlangl@microsoft.com> * Bringing build management settings for release/arm64 in line with debug/arm64 * Fixing arm64 build * Addressed comments * Using winrt constructs on NotificationsLongRunningPlatformImpl :) * Channel Request for unpackaged applications (#1290) * Channel requests for Packaged and Unpackaged applications * Fix comments * Update windowsappsdk_dll to locate PushNotificationsLongRunningTask.ProxyStub headers * Address minor comments * Fix channel.Close API to handle channels when requested through frameworkudk * Address comments and introduce GetAppIdentifier in LRP * Addressing nits * Fixing a few more nits * Update externs.h * Store <processname,appidentifier> pair in windows storage localsettings * Fix minor nits * Remove helpers.h references from PushNotificationsDemoApp * Remove unused APIs * Using WIL type for CoInitializeEx instead of working too hard Co-authored-by: Eric Langlois <erlangl@microsoft.com> Co-authored-by: Paul Purifoy <purifoypaul@microsoft.com> Co-authored-by: Daniel Ayala <14967941+danielayala94@users.noreply.github.com> * Notification delivery + Protocol Activation (#1358) * Protocol Activation skeleton * Further impl, mostly on the LRP * Integration with Foreground logic and some optimizations * Added calls to the LRP from SDK, and LRP improvements * Fixed some issues * Improved payload deserializing * Fixed merging goofs * Addressed feedback + few optimizations * Further improvements and added/removed comments * Addressed build errors after merging * Nit: Removed comment Co-authored-by: Daniel Ayala Co-authored-by: Eric Langlois * Unpackaged notification unit tests (#1340) * Adding initial unpackaged test series * This function now fails silently * This is not expected to work for unpackaged apps Co-authored-by: Eric Langlois <erlangl@microsoft.com> * Ensure errors get logged with telemetry * Workaround UDK include issue (#1364) * Workaround UDK include issue * just duplicating files seem a better approach * Restoring vcproje files * PushNotificationsRT.h was interfering with package creation Co-authored-by: Eric Langlois <erlangl@microsoft.com> * Some nits * Addressed comments * Fixed mistake - Removed comments * Add LRP binaries to CopyFilestoStagingDir * Addressed comments * Merge main to LRP * Telemetry Update for Unpackaged Apps (#1385) * Adding support for unpackaged apps * Using inline statics to cache data * Code cleanup * Code cleanup * Simplifying rethrow code * Addressing feedback * Distinguishing between legacy and modern impl * caching legacy status to it is also available when the call fails * Adding const to better express the intent * Code cleanup * Code cleanup * PR feedback Co-authored-by: Eric Langlois <erlangl@microsoft.com> * Update to latest UDK and undo UDK hack (#1420) Co-authored-by: Eric Langlois <erlangl@microsoft.com> * Fixing WNP_LRP tests (#1401) * Working on tests * Call ReigsterFullTrust in RegisterLongRunningActivator * Removed g_isPackaged with IsActivatorSupported * Addressing nits * Protocol Activation bug fixes (#1406) This PR fixes a couple of issues: - Truncated payloads delivered to the app: the Push payload is now processed correctly to comply with AppLifecycle command line expectations (i.e. truncation happened at the first space character - other activation types are already escaping such characters, now we do it too. Then, the payload is unescaped at GetRawNotificationEventArgs.h). - Payloads with special characters crashed the app: Special characters weren't playing okay with wstring->string conversion at PushNotificationReceivedEventArgs, because WideCharToMultiByte was erroring out with ERROR_INSUFFICIENT_BUFFER. * Use remoteId as const value (#1435) * Upgrade LRP to latest UDK (#1459) * Upgrade LRP to latest UDK * Missed a few checks in the vcproj Co-authored-by: Eric Langlois <erlangl@microsoft.com> * fix bad merge * Update CopyFilesToStagingDir.ps1 * Remove PushNotifications from outer layer of JSON * Adding source link to projects not in main yet (#1515) Co-authored-by: Eric Langlois <erlangl@microsoft.com> * Support foregroundtask registration for packaged applications (#1521) * Support foregroundtask registration for packaged applications * Address comments * Address comments2 * Introduce PushNotificationsUtility.h/cpp * Address comments * Mark PushNotificationUtility.h file to be shared across the PushNotifications directory * Add try/catch around onRawNotificationRecveived * Set foregroundhandled to true by default * Address comments for getappusermodelId API * Address nits * Adress one more nit Co-authored-by: Venkata Sharath Chandra Manchala <vemancha@microsoft.com> * Fix for regression in protocol activation in AppLifecycle (#1558) * Hack to address AppLifecycle regression introduced in main * Code cleanup * Code cleanup * Addressing PR feedback * Adding bare bone unit test to avoid future regression Co-authored-by: Eric Langlois <erlangl@microsoft.com> * LRP E2E Scenario fixes (#1543) * LRP fixes * In progress * Revert "In progress" This reverts commit d5d021a. * Add fixes to E2E testing * Address nits * Added nits from other PR and fixed PushNotificationUtiilty * Use string converter with payload length * Update APITests.cpp * Include AppLifecycle fix for PushArgs * Revert "Include AppLifecycle fix for PushArgs" This reverts commit 33994da. * Addressing nits * Add Verify.h to demo app for helper function * Added TAEF dependencies to Demo/TestApp Co-authored-by: Daniel Ayala <14967941+danielayala94@users.noreply.github.com> * Updating LRP to use latest FrameworkUDK (#1587) Co-authored-by: Eric Langlois <erlangl@microsoft.com> Co-authored-by: Daniel Ayala <joayalag@microsoft.com> Co-authored-by: Sharath Manchala <10109130+sharath2727@users.noreply.github.com> Co-authored-by: Eric Langlois <erlangl@microsoft.com> Co-authored-by: Paul Purifoy <33183370+pmpurifoy@users.noreply.github.com> Co-authored-by: eric langlois <email@ericlanglois.com> Co-authored-by: Paul Purifoy <purifoypaul@microsoft.com> Co-authored-by: Venkata Sharath Chandra Manchala <vemancha@microsoft.com>

Eric Langlois added 4 commits September 8, 2021 06:30

Adding support for unpackaged apps

276445d

Using inline statics to cache data

a555a09

Code cleanup

2dc3d90

Code cleanup

ffdfea5

loneursid added the area-Notifications Toast notification, badges, Live Tiles, push notifications label Sep 8, 2021

ghost added the needs-triage label Sep 8, 2021

loneursid commented Sep 8, 2021

View reviewed changes

Merge branch 'WNP_LRP' into WNP_LRP-Telemetry

c82e14f

loneursid mentioned this pull request Sep 9, 2021

Address Notifications for unpkg apps comments #1372

Closed

Simplifying rethrow code

bc7cfb9

loneursid marked this pull request as ready for review September 9, 2021 14:19

loneursid requested review from DrusTheAxe, danielayala94, jonwis, pmpurifoy and sharath2727 September 9, 2021 14:19

sharath2727 reviewed Sep 10, 2021

View reviewed changes

Merge branch 'WNP_LRP' into WNP_LRP-Telemetry

536958b

sharath2727 reviewed Sep 10, 2021

View reviewed changes

danielayala94 reviewed Sep 10, 2021

View reviewed changes

Comment thread dev/PushNotifications/PushNotificationTelemetry.h Outdated

danielayala94 reviewed Sep 10, 2021

View reviewed changes

Comment thread dev/PushNotifications/PushNotificationTelemetry.h Outdated

Addressing feedback

afde0c3

loneursid requested review from danielayala94 and sharath2727 September 12, 2021 16:52

Eric Langlois added 4 commits September 12, 2021 10:04

Distinguishing between legacy and modern impl

61372e5

caching legacy status to it is also available when the call fails

0b286f0

Merge branch 'WNP_LRP' into WNP_LRP-Telemetry

5098ab4

Adding const to better express the intent

ba7ca4e

Eric Langlois added 2 commits September 13, 2021 09:07

Code cleanup

faf3fc6

Code cleanup

41d4463

danielayala94 approved these changes Sep 14, 2021

View reviewed changes

DrusTheAxe approved these changes Sep 14, 2021

View reviewed changes

PR feedback

790d7e9

loneursid merged commit 256155e into WNP_LRP Sep 14, 2021

loneursid deleted the WNP_LRP-Telemetry branch September 14, 2021 19:53

Conversation

loneursid commented Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loneursid Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loneursid commented Sep 9, 2021

Uh oh!

azure-pipelines Bot commented Sep 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loneursid Sep 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loneursid Sep 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loneursid Sep 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrusTheAxe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

loneursid commented Sep 8, 2021 •

edited

Loading

loneursid Sep 8, 2021 •

edited

Loading

loneursid Sep 12, 2021 •

edited

Loading

loneursid Sep 12, 2021 •

edited

Loading

loneursid Sep 12, 2021 •

edited

Loading