Telemetry reporting improvements #617
Conversation
| // Values are ordered in a typical workflow stages | ||
| enum class ExecutionStage | ||
| { | ||
| None, |
There was a problem hiding this comment.
None [](start = 8, length = 4)
The first phase should probably be something about command line parsing. #Closed
| void operator()(Execution::Context& context) const override; | ||
|
|
||
| private: | ||
| mutable ExecutionStage m_stage; |
There was a problem hiding this comment.
mutable [](start = 8, length = 7)
This doesn't need mutable; you aren't writing to it in a const context. (And the move you are doing is completely unnecessary) #Closed
There was a problem hiding this comment.
Yea, but the compiler is not happy if I don't use std::move() since the context.Add() only takes rvalue.
In reply to: 508324457 [](ancestors = 508324457)
There was a problem hiding this comment.
| @@ -35,6 +35,10 @@ namespace AppInstaller::Logging | |||
| // Used to disable telemetry on the fly. | |||
| std::atomic_bool s_isTelemetryEnabled{ true }; | |||
|
|
|||
| std::string s_executionStage; | |||
There was a problem hiding this comment.
std::string s_executionStage; [](start = 8, length = 29)
I think that while logging a string will be more flexible for adding new stages, it will make processing that data far harder. Instead, we should just log an integer, and the current stages can be ParseArgs = 1000, Discovery = 2000, etc. #Closed
| @@ -117,6 +123,7 @@ namespace AppInstaller::Logging | |||
| "ClientVersion", | |||
| GetActivityId(), | |||
| nullptr, | |||
| TraceLoggingGuid(s_subExecutionId, "SubExecutionId"), | |||
| @@ -140,6 +147,7 @@ namespace AppInstaller::Logging | |||
| "CommandFound", | |||
| GetActivityId(), | |||
| nullptr, | |||
| TraceLoggingGuid(s_subExecutionId, "SubExecutionId"), | |||
| @@ -156,6 +164,7 @@ namespace AppInstaller::Logging | |||
| "CommandSuccess", | |||
| GetActivityId(), | |||
| nullptr, | |||
| TraceLoggingGuid(s_subExecutionId, "SubExecutionId"), | |||
| @@ -35,6 +35,10 @@ namespace AppInstaller::Logging | |||
| // Used to disable telemetry on the fly. | |||
| std::atomic_bool s_isTelemetryEnabled{ true }; | |||
|
|
|||
| std::string s_executionStage; | |||
|
|
|||
| std::atomic<GUID> s_subExecutionId{ GUID_NULL }; | |||
There was a problem hiding this comment.
std::atomic s_subExecutionId{ GUID_NULL }; [](start = 8, length = 48)
Could we instead just use an integer? These don't need to be unique across sessions, so for the same ActivityId, 0 can be the root (like GUID_NULL), and 1+ can be a subexecution. That way we also get an order that might be useful. #Closed
ghost
added
the
Needs-Author-Feedback
| @@ -197,6 +207,24 @@ namespace AppInstaller::CLI::Workflow | |||
| // Inputs: SearchResult | |||
| // Outputs: InstalledPackageVersion | |||
| void GetInstalledPackageVersion(Execution::Context& context); | |||
|
|
|||
| // Searches the source for the specific field as a completion. | |||
There was a problem hiding this comment.
/ Searches the source fo [](start = 5, length = 24)
update comments #Closed
ghost
removed
the
Needs-Author-Feedback
|
|
||
| SubExecutionTelemetryScope::SubExecutionTelemetryScope() | ||
| { | ||
| THROW_HR_IF_MSG(HRESULT_FROM_WIN32(ERROR_INVALID_STATE), s_subExecutionId.exchange(++m_sessionId) != s_RootExecutionId, |
There was a problem hiding this comment.
exchange [](start = 82, length = 8)
Use compare_exchange to not actually change the value if it is not root. #Resolved
| void Add(const typename details::DataMapping<D>::value_t& v) | ||
| { | ||
| auto copy = v; | ||
| m_data[D].emplace<details::DataIndex(D)>(std::forward<typename details::DataMapping<D>::value_t>(std::move(copy))); |
There was a problem hiding this comment.
std::move(copy) [](start = 109, length = 15)
You shouldn't need to move a copy; why can't you just emplace v and have it do the copy? #Resolved
Change
Validation
This should not change the client behavior. Also manually validated basic install and search.
Microsoft Reviewers: Open in CodeFlow