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

Use correct Subexecution id when working on multiple packages #1504

Merged

Conversation

ashpatil-msft
Copy link
Contributor

@ashpatil-msft ashpatil-msft commented Sep 24, 2021


Issue:

  • During import and upgrade flows, the sub execution id that was being used was not consistent for all events belonging to the same package as new ids were being generated in different phases.

Fix:

  • This PR has changes that stores the sub execution id that was introduced for each package in search phase and re-uses it in the install phase, so that all events belonging to the same package have the same sub execution id.

Tests:

  • Manually tested and verified the events sent for import and upgrade scenarios. Here are the sub exec ids used for upgrade events:
Console output when finding the package:
 Upgrade App name: Microsoft.VisualStudio.2019.Enterprise Sub exec id: 2
 Upgrade App name: Microsoft.WingetCreate Sub exec id: 55
 Upgrade App name: Microsoft.Teams Sub exec id: 81 
Console output when installing the package:
 Install App name: Microsoft.VisualStudio.2019.Enterprise Sub exec id: 2
 Install App name: Microsoft.WingetCreate Sub exec id: 55
 Install App name: Microsoft.Teams Sub exec id: 81

Microsoft Reviewers: Open in CodeFlow

@ashpatil-msft ashpatil-msft requested a review from a team as a code owner September 24, 2021 17:48
@github-actions
Copy link

github-actions bot commented Sep 24, 2021

@check-spelling-bot Report

Unrecognized words, please review:

  • subexecution
Previously acknowledged words that are now absent ATL Bluetooth contosa contosainstaller convertto doctordns DSL etail globals IInstalled Jaifroid JDKs mikefrobbins mytool Nieto powershellexplained xbox
To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:ashpatil-msft/winget-cli.git repository
on the user/ashpatil/fixsubexecutionid branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/winget-cli/issues/comments/926814080" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u

In reply to: 926814080

packageRequest.Scope,
subExecution.GetCurrentSubExecutionId()};

packagesToInstall.emplace_back(std::move(package));
Copy link
Member

@JohnMcPMS JohnMcPMS Sep 24, 2021

Choose a reason for hiding this comment

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

nit: Why change this? It isn't really a big deal perf-wise, but previously it was direct construction and now it is construct and move. #Resolved


// Use this subexecution id when installing this package so that
// install telemetry is captured with the same sub execution id as other events in Search phase.
uint32_t PackageSubExecutionId;
Copy link
Contributor

@yao-msft yao-msft Sep 25, 2021

Choose a reason for hiding this comment

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

PackageSubExecutionId

nit: = 0 #Resolved

SubExecutionTelemetryScope(const SubExecutionTelemetryScope&) = delete;
SubExecutionTelemetryScope& operator=(const SubExecutionTelemetryScope&) = delete;

SubExecutionTelemetryScope(SubExecutionTelemetryScope&&) = default;
SubExecutionTelemetryScope& operator=(SubExecutionTelemetryScope&&) = default;

uint32_t GetCurrentSubExecutionId();
Copy link
Contributor

@yao-msft yao-msft Sep 25, 2021

Choose a reason for hiding this comment

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

;

nit: const ? #Resolved

@ashpatil-msft ashpatil-msft merged commit 80fcb9b into microsoft:master Sep 27, 2021
JohnMcPMS pushed a commit that referenced this pull request Oct 25, 2021
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.

3 participants