Log Com invocation startup telemetry and delay auto update time when invoked from explorer #3665
Conversation
| try | ||
| { | ||
| auto [hrGetCallerId, callerProcessId] = GetCallerProcessId(); | ||
| THROW_IF_FAILED(hrGetCallerId); |
There was a problem hiding this comment.
Don't use exceptions for control flow.
| try | ||
| // OutOfProc case, we check for explorer.exe | ||
| auto callerNameWide = AppInstaller::Utility::ConvertToUTF16(GetCallerName()); | ||
| auto processName = AppInstaller::Utility::ConvertToUTF8(std::filesystem::path{ callerNameWide }.filename().wstring()); |
There was a problem hiding this comment.
Please move this detection to a shared utility function as I suspect we will need it again (and I want to make it very explicit that we are doing this).
At the same time, wrap it in a try/catch so that we can ensure that an unexpected caller value being set doesn't blow up path parsing or something. The values we are targeting should be ok, and so any exception can be treated as not the case we are looking for.
| source = m_sourceReference; | ||
| source.SetCaller(GetCallerName()); | ||
| source.SetCaller(callerName); |
There was a problem hiding this comment.
Don't you need to call SetBackgroundUpdateInterval here? I'm not seeing it being set anywhere else...
| { | ||
| // See if caller name is set by caller | ||
| static auto callerName = GetComCallerName(""); | ||
| static constexpr winrt::Windows::Foundation::TimeSpan s_PackageCatalogUpdateIntervalDelay = 168h; //1 week |
There was a problem hiding this comment.
We might want to randomize this a bit as well, at least + 0/24 hours (worth of seconds or smaller) but maybe even up to a week extra. That will hopefully prevent this from causing time based scheduled tasks to align on updates.
| @@ -143,4 +145,26 @@ namespace winrt::Microsoft::Management::Deployment::implementation | |||
|
|
|||
| return callerName; | |||
| } | |||
|
|
|||
| bool ShouldDelayBackgroundUpdateByCaller() | |||
There was a problem hiding this comment.
I really meant with a name that was like IsBackgroundProcessForPolicy or something.
| m_packageCatalogBackgroundUpdateInterval = s_PackageCatalogUpdateIntervalDelay; | ||
| // Add a bit of randomness to the default interval time | ||
| std::default_random_engine randomEngine(std::random_device{}()); | ||
| std::uniform_int_distribution<long long> distribution(0, 168); |
There was a problem hiding this comment.
We really want to spread this out, so multiple this by 3600 for second resolution.
| auto previousThreadGlobals = m_threadGlobals.SetForCurrentThread(); | ||
| // Immediately reset as we only want the thread globals for logging within this object. | ||
| previousThreadGlobals.reset(); | ||
| m_threadGlobals.GetTelemetryLogger().SetCaller(GetCallerName()); |
There was a problem hiding this comment.
Make sure that this doesn't send a summary event. There is a bool that disables it as I remember.
This comment has been minimized.
This comment has been minimized.
…invoked from explorer (microsoft#3665) - Add telemetry event for PackageManager class creation - Delay source update time to 7 days by default when invoked from explorer - Also fixes Source agreements related crash found when doing the above work
…invoked from explorer (#3665) (#3684) - Add telemetry event for PackageManager class creation - Delay source update time to 7 days by default when invoked from explorer - Also fixes Source agreements related crash found when doing the above work Co-authored-by: yao-msft <[email protected]>
Microsoft Reviewers: Open in CodeFlow