Skip to content
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.

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

Make telemetry logger per context and send summary event on destruction #1718

Merged
merged 10 commits into from
Nov 30, 2021

Conversation

yao-msft
Copy link
Contributor

@yao-msft yao-msft commented Nov 16, 2021

Change

  • Change TelemetryTraceLogger to be per context
  • Add parent activity id concept to replace the subExecutionId
  • Send summary event upon each TelemetryTraceLogger destruction

Validation

Due to lack of test infra support for telemetry, I manually run and see telemetry events and also set breakpoints to verify the summary events.

Microsoft Reviewers: Open in CodeFlow

@yao-msft yao-msft requested a review from a team as a code owner November 16, 2021 19:37
src/AppInstallerCommonCore/ThreadGlobals.cpp Outdated Show resolved Hide resolved
src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h Outdated Show resolved Hide resolved
src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h Outdated Show resolved Hide resolved
src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h Outdated Show resolved Hide resolved
src/AppInstallerCommonCore/Public/AppInstallerTelemetry.h Outdated Show resolved Hide resolved
src/AppInstallerCommonCore/AppInstallerTelemetry.cpp Outdated Show resolved Hide resolved
src/AppInstallerCLICore/Core.cpp Show resolved Hide resolved
src/AppInstallerCLICore/Commands/CompleteCommand.cpp Outdated Show resolved Hide resolved
src/AppInstallerCLICore/Core.cpp Outdated Show resolved Hide resolved
src/AppInstallerCLICore/ExecutionContext.cpp Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback Issue needs attention from issue or PR author label Nov 17, 2021
@ghost ghost removed the Needs-Author-Feedback Issue needs attention from issue or PR author label Nov 19, 2021

// Enable all logging for this phase; we will update once we have the arguments
Logging::Log().EnableChannel(Logging::Channel::All);
Logging::Log().SetLevel(Logging::Level::Info);
Logging::AddFileLogger();
Logging::EnableWilFailureTelemetry();

#ifndef AICLI_DISABLE_TEST_HOOKS
Copy link
Member

@JohnMcPMS JohnMcPMS Nov 20, 2021

Choose a reason for hiding this comment

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

This should be done before everything but COM initialization. #Resolved

@@ -184,6 +184,7 @@ namespace AppInstaller::CLI::Execution
void Context::SetTerminationHR(HRESULT hr)
{
m_terminationHR = hr;
m_isTerminated = FAILED(hr);
Copy link
Member

@JohnMcPMS JohnMcPMS Nov 20, 2021

Choose a reason for hiding this comment

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

Suggested change
m_isTerminated = FAILED(hr);
m_isTerminated = true;

Should stay consistent with Terminate. #Resolved

@ghost ghost added the Needs-Author-Feedback Issue needs attention from issue or PR author label Nov 20, 2021
@ghost ghost removed the Needs-Author-Feedback Issue needs attention from issue or PR author label Nov 20, 2021
@yao-msft yao-msft merged commit cc2dd14 into microsoft:master Nov 30, 2021
@yao-msft yao-msft deleted the telemetrysummary2 branch November 30, 2021 20:25
Trenly added a commit to Trenly/winget-cli that referenced this pull request Mar 22, 2024
411a10915 mint 2.10.19
096a9a1bb Merge pull request microsoft#1782 from microsoft/fix_uri_parsing
006271f6a make Uri.is_host_loopback() only return true for localhost and 127.0.0.1 exactly
9c654889e Remove email list from the readme
31e7feacc Merge pull request microsoft#1718 from dashanji/Fix-typo
e1b6a8e61 Merge pull request microsoft#1711 from Fighter19/pr-fix-safeint3
c5dcbb5bb Merge pull request microsoft#1717 from microsoft/users/GitHubPolicyService/f2ee14d6-8d6b-4313-b754-3880e015f7c3
98ee36dac Fix typo
a57f45918 Microsoft mandatory file
3308d9728 Fix likely typo in SafeInt3.hpp, that results in error with clang 15
07cf58910 Merge pull request microsoft#1429 from NN---/fix/value_int_ctor
3eac925ad Update Release/include/cpprest/json.h
06363bc78 Update Release/include/cpprest/json.h
bfe348779 Merge pull request #1577 from JvdGlind/hidden_visibility_support_macos
0ddc61829 Clarify cpprestsdk level of support
804448058 export http_exception for non Windows builds
d9d7f5ed4 Merge pull request microsoft#1496 from icherniukh/oauth2_client_credentials
8ae5da616 Update oauth2.h
708a5df2b Add support for oauth2 using only client credentials
5408f1dc9 Add constructor from all integer types.

git-subtree-dir: src/cpprestsdk/cpprestsdk
git-subtree-split: 411a109150b270f23c8c97fa4ec9a0a4a98cdecf
Trenly added a commit to Trenly/winget-cli that referenced this pull request Mar 22, 2024
411a10915 mint 2.10.19
096a9a1bb Merge pull request microsoft#1782 from microsoft/fix_uri_parsing
006271f6a make Uri.is_host_loopback() only return true for localhost and 127.0.0.1 exactly
9c654889e Remove email list from the readme
31e7feacc Merge pull request microsoft#1718 from dashanji/Fix-typo
e1b6a8e61 Merge pull request microsoft#1711 from Fighter19/pr-fix-safeint3
c5dcbb5bb Merge pull request microsoft#1717 from microsoft/users/GitHubPolicyService/f2ee14d6-8d6b-4313-b754-3880e015f7c3
98ee36dac Fix typo
a57f45918 Microsoft mandatory file
3308d9728 Fix likely typo in SafeInt3.hpp, that results in error with clang 15
07cf58910 Merge pull request microsoft#1429 from NN---/fix/value_int_ctor
3eac925ad Update Release/include/cpprest/json.h
06363bc78 Update Release/include/cpprest/json.h
bfe348779 Merge pull request #1577 from JvdGlind/hidden_visibility_support_macos
0ddc61829 Clarify cpprestsdk level of support
804448058 export http_exception for non Windows builds
d9d7f5ed4 Merge pull request microsoft#1496 from icherniukh/oauth2_client_credentials
8ae5da616 Update oauth2.h
708a5df2b Add support for oauth2 using only client credentials
5408f1dc9 Add constructor from all integer types.

git-subtree-dir: src/cpprestsdk/cpprestsdk
git-subtree-split: 411a109150b270f23c8c97fa4ec9a0a4a98cdecf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants