Skip to content

Move telemetry start to earliest point in DeploymentManager public APIs #6319

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

Draft
lauren-ciha wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Move telemetry start to earliest point in DeploymentManager public APIs #6319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
170 changes: 164 additions & 6 deletions dev/Deployment/DeploymentManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,94 @@ inline void Initialize_StopSuccessActivity(
initializeActivityContext.GetUseExistingPackageIfHigherVersion());
}

inline void GetStatus_StopSuccessActivity(
::WindowsAppRuntime::Deployment::Activity::Context& activityContext,
const winrt::Microsoft::Windows::ApplicationModel::WindowsAppRuntime::DeploymentStatus& deploymentStatus)
{
activityContext.GetActivity().StopWithResult(
S_OK,
static_cast<UINT32>(0),
static_cast<PCSTR>(nullptr),
static_cast<unsigned int>(0),
static_cast<PCWSTR>(nullptr),
static_cast<PCSTR>(nullptr),
static_cast<UINT32>(deploymentStatus),
static_cast<UINT32>(::WindowsAppRuntime::Deployment::Activity::DeploymentStage::None),
static_cast<PCWSTR>(nullptr),
S_OK,
static_cast<PCWSTR>(nullptr),
GUID{},
false /*useExistingPackageIfHigherVersion*/);
Comment on lines +53 to +66
}

namespace winrt::Microsoft::Windows::ApplicationModel::WindowsAppRuntime::implementation
{
static bool s_isInitialized{ false };
static wil::srwlock m_isInitializedLock;

winrt::Microsoft::Windows::ApplicationModel::WindowsAppRuntime::DeploymentResult DeploymentManager::GetStatus()
{
FAIL_FAST_HR_IF(HRESULT_FROM_WIN32(APPMODEL_ERROR_NO_PACKAGE), !AppModel::Identity::IsPackagedProcess());
return GetStatus(GetCurrentFrameworkPackageFullName());
// Start telemetry FIRST so we capture all failures, including unpackaged process errors.
auto& activityContext{ ::WindowsAppRuntime::Deployment::Activity::Context::Get() };

// Start activity immediately with safe defaults - the WIL callback will capture any errors.
activityContext.GetActivity().Start(
Copy link
Copy Markdown
Member

@Scottj1s Scottj1s Mar 23, 2026

Choose a reason for hiding this comment

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

Did the original code even work? The Start method constructs and returns the static singleton, which should then be captured via SetActivity for subsequent tracing via GetActivity. See the InstallActivityContext for comparison. In any case, we're foregoing the benefits of using TraceLoggingProvider's nested RAII TraceLoggingActivity with this pattern, instead requiring explicit Stop calls. We could create an RAII wrapper to ensure Stop is called on scope exit, but much simpler to just use TraceLoggingActivity as designed:

{
// activity scope
auto& activity = WindowsAppRuntimeDeployment_TraceLogger::::Start(...);
try
{
// do stuff ...
}
catch
{
activity.StopWithResult(...);
}
// activity goes out of scope, conditionally calls Stop
}

The same goes for WindowsAppRuntimeInstaller_TraceLogger::Install, etc.

This PR is also far too intrusive and obfuscates the real functionality with all the tracing calls. GetStatus_StopSuccessActivity is a good concept to shrink that cognitive overhead. I'd ask the bot to redesign the code to minimize the intrusion, and to leverage RAII.

Copy link
Copy Markdown
Member Author

@lauren-ciha lauren-ciha Mar 23, 2026

Choose a reason for hiding this comment

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

I agree. This was a first pass attempt at resolving the issue. @agniuks took another try at the issue in this PR: #6323. I'll be reviewing it soon.

false /*forceDeployment*/,
Security::IntegrityLevel::IsElevated(),
true /*isPackagedProcess - assume true, we check below*/,
false /*isFullTrustPackage*/,
0 /*integrityLevel*/,
false /*isRepair*/,
true /*isGetStatus*/);

// Now check if packaged - errors are captured by telemetry
const bool isPackagedProcess{ AppModel::Identity::IsPackagedProcess() };
Comment on lines +79 to +90
if (!isPackagedProcess)
{
// Stop telemetry and fail fast
activityContext.GetActivity().StopWithResult(
HRESULT_FROM_WIN32(APPMODEL_ERROR_NO_PACKAGE),
static_cast<UINT32>(wil::FailureType::FailFast),
__FILE__,
__LINE__,
L"GetStatus called from unpackaged process",
static_cast<PCSTR>(nullptr),
static_cast<UINT32>(winrt::Microsoft::Windows::ApplicationModel::WindowsAppRuntime::DeploymentStatus::Unknown),
static_cast<UINT32>(::WindowsAppRuntime::Deployment::Activity::DeploymentStage::None),
L"IsPackagedProcess",
S_OK,
static_cast<PCWSTR>(nullptr),
GUID{},
false);
FAIL_FAST_HR(HRESULT_FROM_WIN32(APPMODEL_ERROR_NO_PACKAGE));
}

try
{
auto result = GetStatus(GetCurrentFrameworkPackageFullName());
GetStatus_StopSuccessActivity(activityContext, result.Status());
return result;
}
catch (...)
{
// Capture exception in telemetry before re-throwing
const HRESULT hr = wil::ResultFromCaughtException();
activityContext.GetActivity().StopWithResult(
hr,
static_cast<UINT32>(wil::FailureType::Exception),
static_cast<PCSTR>(nullptr),
static_cast<unsigned int>(0),
static_cast<PCWSTR>(nullptr),
static_cast<PCSTR>(nullptr),
static_cast<UINT32>(winrt::Microsoft::Windows::ApplicationModel::WindowsAppRuntime::DeploymentStatus::Unknown),
static_cast<UINT32>(::WindowsAppRuntime::Deployment::Activity::DeploymentStage::None),
L"GetStatus",
S_OK,
static_cast<PCWSTR>(nullptr),
GUID{},
false);
throw;
}
}

winrt::Microsoft::Windows::ApplicationModel::WindowsAppRuntime::DeploymentResult DeploymentManager::Initialize()
Expand All @@ -66,13 +145,88 @@ namespace winrt::Microsoft::Windows::ApplicationModel::WindowsAppRuntime::implem
winrt::Microsoft::Windows::ApplicationModel::WindowsAppRuntime::DeploymentResult DeploymentManager::Initialize(
winrt::Microsoft::Windows::ApplicationModel::WindowsAppRuntime::DeploymentInitializeOptions const& deploymentInitializeOptions)
{
return Initialize(GetCurrentFrameworkPackageFullName(), deploymentInitializeOptions);
// Start telemetry FIRST so we capture all failures, including framework discovery errors.
auto& initializeActivityContext{ ::WindowsAppRuntime::Deployment::Activity::Context::Get() };

// Start activity immediately with safe defaults - the WIL callback will capture any errors
// that occur during GetCurrentFrameworkPackageFullName() or subsequent operations.
initializeActivityContext.GetActivity().Start(
deploymentInitializeOptions.ForceDeployment(),
false /*isElevated - actual value determined later*/,
true /*isPackagedProcess - assume true as safe default*/,
false /*isFullTrustPackage - determined later*/,
0 /*integrityLevel - determined later*/,
false /*isRepair*/,
false /*isGetStatus*/);

try
{
return Initialize(GetCurrentFrameworkPackageFullName(), deploymentInitializeOptions);
Comment on lines +162 to +164
Copy link
Copy Markdown
Contributor

@guimafelipe guimafelipe Mar 19, 2026

Choose a reason for hiding this comment

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

With this new try catch, do we still need the one inside the Initialize method?

}
Comment on lines +162 to +165
catch (...)
{
// Capture exception in telemetry before re-throwing
const HRESULT hr = wil::ResultFromCaughtException();
initializeActivityContext.GetActivity().StopWithResult(
Comment on lines +148 to +170
hr,
static_cast<UINT32>(wil::FailureType::Exception),
static_cast<PCSTR>(nullptr),
static_cast<unsigned int>(0),
static_cast<PCWSTR>(nullptr),
static_cast<PCSTR>(nullptr),
static_cast<UINT32>(winrt::Microsoft::Windows::ApplicationModel::WindowsAppRuntime::DeploymentStatus::PackageInstallRequired),
static_cast<UINT32>(::WindowsAppRuntime::Deployment::Activity::DeploymentStage::DiscoverFramework),
L"GetCurrentFrameworkPackageFullName",
Comment on lines +177 to +179
S_OK,
static_cast<PCWSTR>(nullptr),
GUID{},
false);
throw;
}
}

winrt::Microsoft::Windows::ApplicationModel::WindowsAppRuntime::DeploymentResult DeploymentManager::Repair()
{
winrt::Microsoft::Windows::ApplicationModel::WindowsAppRuntime::DeploymentInitializeOptions options{};
return Initialize(GetCurrentFrameworkPackageFullName(), options, true);

// Start telemetry FIRST so we capture all failures, including framework discovery errors.
auto& initializeActivityContext{ ::WindowsAppRuntime::Deployment::Activity::Context::Get() };

// Start activity immediately with safe defaults - the WIL callback will capture any errors
// that occur during GetCurrentFrameworkPackageFullName() or subsequent operations.
initializeActivityContext.GetActivity().Start(
options.ForceDeployment(),
false /*isElevated - actual value determined later*/,
true /*isPackagedProcess - assume true as safe default*/,
false /*isFullTrustPackage - determined later*/,
0 /*integrityLevel - determined later*/,
true /*isRepair*/,
false /*isGetStatus*/);
Comment on lines +197 to +204

try
{
return Initialize(GetCurrentFrameworkPackageFullName(), options, true);
}
Comment on lines +206 to +209
catch (...)
{
// Capture exception in telemetry before re-throwing
const HRESULT hr = wil::ResultFromCaughtException();
initializeActivityContext.GetActivity().StopWithResult(
hr,
static_cast<UINT32>(wil::FailureType::Exception),
static_cast<PCSTR>(nullptr),
static_cast<unsigned int>(0),
static_cast<PCWSTR>(nullptr),
static_cast<PCSTR>(nullptr),
static_cast<UINT32>(winrt::Microsoft::Windows::ApplicationModel::WindowsAppRuntime::DeploymentStatus::PackageInstallRequired),
static_cast<UINT32>(::WindowsAppRuntime::Deployment::Activity::DeploymentStage::DiscoverFramework),
L"GetCurrentFrameworkPackageFullName",
S_OK,
static_cast<PCWSTR>(nullptr),
GUID{},
false);
throw;
}
}

std::wstring ExtractFormattedVersionTag(const std::wstring& versionTag)
Expand Down Expand Up @@ -217,8 +371,12 @@ namespace winrt::Microsoft::Windows::ApplicationModel::WindowsAppRuntime::implem
initializeActivityContext.SetIsFullTrustPackage();
}

initializeActivityContext.GetActivity().Start(deploymentInitializeOptions.ForceDeployment(), Security::IntegrityLevel::IsElevated(),
isPackagedProcess, initializeActivityContext.GetIsFullTrustPackage(), integrityLevel, isRepair);
// Only start telemetry if not already running (public APIs start it before calling us).
if (!initializeActivityContext.GetActivity().IsRunning())
{
initializeActivityContext.GetActivity().Start(deploymentInitializeOptions.ForceDeployment(), Security::IntegrityLevel::IsElevated(),
isPackagedProcess, initializeActivityContext.GetIsFullTrustPackage(), integrityLevel, isRepair);
}
Comment on lines +374 to +379
Copy link
Copy Markdown
Contributor

@guimafelipe guimafelipe Mar 19, 2026

Choose a reason for hiding this comment

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

Is this needed? Can't we just know for sure that telemetry is already started?

Copy link
Copy Markdown
Member

@Scottj1s Scottj1s Mar 23, 2026

Choose a reason for hiding this comment

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

RAII should guarantee initilaization


// DeploymentManager API requires a packaged process?
winrt::Microsoft::Windows::ApplicationModel::WindowsAppRuntime::DeploymentResult deploymentResult{ winrt::Microsoft::Windows::ApplicationModel::WindowsAppRuntime::DeploymentStatus::Unknown, S_OK };
Expand Down
5 changes: 3 additions & 2 deletions dev/Deployment/DeploymentTraceLogging.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class WindowsAppRuntimeDeployment_TraceLogger final : public wil::TraceLoggingPr
public:

BEGIN_COMPLIANT_CRITICAL_DATA_ACTIVITY_CLASS(Initialize, PDT_ProductAndServicePerformance);
void StartActivity(bool forceDeployment, bool isElevated, bool isPackagedProcess, bool isFullTrustPackage, DWORD integrityLevel, bool isRepair)
void StartActivity(bool forceDeployment, bool isElevated, bool isPackagedProcess, bool isFullTrustPackage, DWORD integrityLevel, bool isRepair, bool isGetStatus = false)
{
Comment on lines +23 to 24
Copy link
Copy Markdown
Contributor

@guimafelipe guimafelipe Mar 19, 2026

Choose a reason for hiding this comment

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

Instead of passing these two bools, we could just use an enum here.

// Clear the process-wide callback set in Start
wil::SetResultLoggingCallback(nullptr);
Expand All @@ -35,7 +35,8 @@ class WindowsAppRuntimeDeployment_TraceLogger final : public wil::TraceLoggingPr
TraceLoggingValue(isPackagedProcess, "isPackagedProcess"),
TraceLoggingValue(isFullTrustPackage, "isFullTrustPackage"),
TraceLoggingValue(integrityLevel, "integrityLevel"),
TraceLoggingValue(isRepair, "isRepairAPI"));
TraceLoggingValue(isRepair, "isRepairAPI"),
TraceLoggingValue(isGetStatus, "isGetStatusAPI"));
}
void StopWithResult(
HRESULT hresult,
Expand Down
Loading