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

(ENGTASKS-3693) Prototype updating user agent with process information #3460

Merged
merged 10 commits into from
Nov 7, 2024

Conversation

AdmiringWorm
Copy link
Member

@AdmiringWorm AdmiringWorm commented Jun 7, 2024

Description Of Changes

  • Adds information to the user agent provided by NuGet when querying repositories, showing the process that called choco.exe as well as the root process of the tree (filtering out common shells, terminal emulators, and explorer.exe).

This PR also includes a prototype changes of an investigation where we include additional information about the current process tree that choco.exe is part of, including additional information about what the current process is when needed.

Additionally, benchmarks have also been added as part of this investigation.

Below is the results from these benchmarks that was ran on a dedicated computer (This had been ran multiple times, but only one result is included).

BenchmarkDotNet v0.13.12, Windows 11 (10.0.22631.3593/23H2/2023Update/SunValley3)
AMD Ryzen 7 2700X, 1 CPU, 16 logical and 8 physical cores
[Host] : .NET Framework 4.8.1 (4.8.9241.0), X86 LegacyJIT
Job-NBIXUS : .NET Framework 4.8.1 (4.8.9241.0), X64 LegacyJIT

Jit=LegacyJit Platform=X64 Runtime=.NET Framework 4.8

Method Mean Error StdDev Ratio Gen0 Gen1 Gen2 Allocated Alloc Ratio
GetParentProcessDocumentedPinvoke 25.297 ms 0.0724 ms 0.0677 ms 0.35 - - - 89.5 KB 0.010
GetParentProcessFilteredDocumentedPinvoke 26.714 ms 0.0955 ms 0.0894 ms 0.37 31.2500 - - 186.25 KB 0.020
GetParentProcessFilteredManaged 76.386 ms 0.1347 ms 0.1260 ms 1.06 2000.0000 1000.0000 1000.0000 9499.4 KB 1.014
GetParentProcessFilteredUndocumentedPinvoke 11.621 ms 0.0451 ms 0.0422 ms 0.16 31.2500 - - 155.88 KB 0.017
GetParentProcessManaged 72.269 ms 0.2458 ms 0.2179 ms 1.00 2000.0000 1000.0000 1000.0000 9371.36 KB 1.000
GetParentProcessTreeDocumentedPinvoke 27.000 ms 0.0896 ms 0.0838 ms 0.37 31.2500 - - 231.5 KB 0.025
GetParentProcessTreeManaged 75.331 ms 0.2134 ms 0.1997 ms 1.04 2000.0000 1000.0000 1000.0000 9370.23 KB 1.000
GetParentProcessTreeUndocumentedPinvoke 11.561 ms 0.1412 ms 0.1569 ms 0.16 46.8750 15.6250 - 199.76 KB 0.021
GetParentProcessUndocumentedPinvoke 7.860 ms 0.0649 ms 0.0607 ms 0.11 - - - 59.5 KB 0.006
GetParentProcessTreeImplemented 11.416 ms 0.0385 ms 0.0360 ms 0.16 46.8750 15.6250 - 199.76 KB 0.021
Method tree Mean Error StdDev Median Ratio RatioSD Gen0 Allocated Alloc Ratio
GetFirstFilteredProcessName chocolatey.benchmark 1.9503 ns 0.0040 ns 0.0033 ns 1.9511 ns ? ? - - ?
GetFirstProcessName chocolatey.benchmark 1.9536 ns 0.0056 ns 0.0047 ns 1.9540 ns ? ? - - ?
GetLastFilteredProcessName chocolatey.benchmark 1.9520 ns 0.0036 ns 0.0033 ns 1.9525 ns ? ? - - ?
GetLastProcessName chocolatey.benchmark 1.9546 ns 0.0079 ns 0.0066 ns 1.9531 ns ? ? - - ?
GetProcessesList chocolatey.benchmark 0.0002 ns 0.0008 ns 0.0007 ns 0.0000 ns ? ? - - ?
GetFirstFilteredProcessName choco(...)minal [63] 178.0415 ns 0.7922 ns 0.7411 ns 178.0618 ns ? ? 0.0153 64 B ?
GetFirstProcessName choco(...)minal [63] 2.9928 ns 0.0036 ns 0.0034 ns 2.9919 ns ? ? - - ?
GetLastFilteredProcessName choco(...)minal [63] 178.9408 ns 0.3029 ns 0.2685 ns 178.9092 ns ? ? 0.0153 64 B ?
GetLastProcessName choco(...)minal [63] 3.7814 ns 0.0060 ns 0.0056 ns 3.7821 ns ? ? - - ?
GetProcessesList choco(...)minal [63] 0.0000 ns 0.0000 ns 0.0000 ns 0.0000 ns ? ? - - ?
GetFirstFilteredProcessName choco(...)lorer [58] 163.7587 ns 0.3715 ns 0.3475 ns 163.8501 ns ? ? 0.0172 72 B ?
GetFirstProcessName choco(...)lorer [58] 2.9863 ns 0.0026 ns 0.0023 ns 2.9866 ns ? ? - - ?
GetLastFilteredProcessName choco(...)lorer [58] 193.6758 ns 0.5614 ns 0.4688 ns 193.5690 ns ? ? 0.0248 104 B ?
GetLastProcessName choco(...)lorer [58] 3.7737 ns 0.0127 ns 0.0112 ns 3.7711 ns ? ? - - ?
GetProcessesList choco(...)lorer [58] 0.0000 ns 0.0000 ns 0.0000 ns 0.0000 ns ? ? - - ?
GetFirstFilteredProcessName choco(...)Tabby [37] 189.7154 ns 1.0933 ns 0.9692 ns 189.5110 ns ? ? 0.0248 104 B ?
GetFirstProcessName choco(...)Tabby [37] 2.7607 ns 0.0054 ns 0.0045 ns 2.7617 ns ? ? - - ?
GetLastFilteredProcessName choco(...)Tabby [37] 166.6200 ns 0.3285 ns 0.3073 ns 166.6799 ns ? ? 0.0172 72 B ?
GetLastProcessName choco(...)Tabby [37] 3.3230 ns 0.0056 ns 0.0046 ns 3.3236 ns ? ? - - ?
GetProcessesList choco(...)Tabby [37] 0.0000 ns 0.0000 ns 0.0000 ns 0.0000 ns ? ? - - ?

Motivation and Context

To investigate possibilies that we have.

Testing

  • Run choco search chocolatey --debug and verify that the process tree and user agent including the root and calling process are shown in the debug logs.
  • Further verification can be done with wireshark or similar to intercept the network calls and verify the user agent is in fact sent as expected.

Operating Systems Testing

  • Windows 10
  • Windows 11

Change Types Made

  • Bug fix (non-breaking change).
  • Feature / Enhancement (non-breaking change).
  • Breaking change (fix or feature that could cause existing functionality to change).
  • Documentation changes.
  • PowerShell code changes.
  • Investigation

Change Checklist

  • Requires a change to the documentation.
  • Documentation has been updated.
  • Tests to cover my changes, have been added.
  • All new and existing tests passed?
  • PowerShell code changes: PowerShell v2 compatibility checked?

Related Issue

Fixes #3526

@AdmiringWorm AdmiringWorm requested a review from gep13 June 7, 2024 13:07
@pauby
Copy link
Member

pauby commented Oct 4, 2024

Raised issue #3526 for this.

@gep13 gep13 linked an issue Oct 24, 2024 that may be closed by this pull request
2 tasks
@vexx32 vexx32 force-pushed the benchmark branch 3 times, most recently from 0f118c8 to b62b5e0 Compare October 31, 2024 15:43
@vexx32 vexx32 marked this pull request as ready for review October 31, 2024 15:57
@vexx32 vexx32 requested a review from a team as a code owner October 31, 2024 15:57
@vexx32
Copy link
Member

vexx32 commented Oct 31, 2024

@corbob I think this is ready for review at this point.

Copy link
Member

@corbob corbob left a comment

Choose a reason for hiding this comment

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

Just a few comments and I seem to recall a few changes...

corbob
corbob previously approved these changes Nov 4, 2024
@vexx32 vexx32 force-pushed the benchmark branch 3 times, most recently from d7c879f to 5a218a4 Compare November 5, 2024 19:30
@corbob corbob marked this pull request as draft November 6, 2024 01:52
@corbob
Copy link
Member

corbob commented Nov 6, 2024

I've run the tests in TestKitchen and am seeing failures that I wasn't seeing when running locally. I'm going to mark this as draft for now so we don't inadvertently merge it.

Copy link
Member

@gep13 gep13 left a comment

Choose a reason for hiding this comment

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

From a change to the CODEOWNERS files, this LGTM, however, I have left a couple comments elsewhere that I think need to be addressed.

src/chocolatey.benchmark/helpers/ManagedProcessHelper.cs Outdated Show resolved Hide resolved
src/chocolatey.benchmark/helpers/ManagedProcessHelper.cs Outdated Show resolved Hide resolved
src/chocolatey.benchmark/helpers/PinvokeProcessHelper.cs Outdated Show resolved Hide resolved
src/chocolatey.benchmark/helpers/PinvokeProcessHelper.cs Outdated Show resolved Hide resolved
src/chocolatey.benchmark/helpers/PinvokeProcessHelper.cs Outdated Show resolved Hide resolved
src/chocolatey.console/Program.cs Outdated Show resolved Hide resolved
src/chocolatey/infrastructure.app/nuget/NugetCommon.cs Outdated Show resolved Hide resolved
src/chocolatey/infrastructure/information/ProcessTree.cs Outdated Show resolved Hide resolved
@corbob corbob marked this pull request as ready for review November 7, 2024 15:42
Copy link
Member

@gep13 gep13 left a comment

Choose a reason for hiding this comment

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

LGTM!

@gep13
Copy link
Member

gep13 commented Nov 7, 2024

This branch is out of date with the develop branch, so not in a position to merge this just now, but I am happy to activate the Enable auto-merge button.

@gep13 gep13 enabled auto-merge November 7, 2024 16:18
AdmiringWorm and others added 10 commits November 7, 2024 09:28
Work the ProcessTree into a service that can be pulled in to the
NugetCommon library in order to provide a more informative user agent
when querying repositories.
The primary method is quickest, but as it is an unstable API, we have a
need to provide a fallback that will be able to be used if the DLL is
missing/removed, or if the entry point is later removed.

Of the stable options, this p/invoke method seems to be the next best
option to work with.

Crucially, these p/invokes need to be provided by different types, so
that if there is a TypeLoadException from a failed p/invoke, we can
still attempt the fallback.

Also, added a couple more exclusions for the choco.exe shim and for
winlogon, which are not useful to include.
We need to define values for ReleaseOfficial for the benchmark project
or the Cake build complains.

Also added Chocolatey.PowerShell to the slnf in the repo, since that was
missing.
RuntimeInformation is shadowed by Mono which means that build breaks and
because all of the namespaces are the same under Mono's version, that
name is not usable in our build currently.

We can reuse our existing Platform.GetPlatform() helper here instead,
rather than trying to disentangle exactly why this build configuration
is not particularly functional.
These processes are not particularly useful to note as they are common
terminal emulators, so we can exclude them when looking at the process
tree.
Simplify some naming, also add a constant (and comment) to clarify what
some of the p/invoke nonsense is doing.
@gep13 gep13 merged commit 475f80b into chocolatey:develop Nov 7, 2024
5 checks passed
@AdmiringWorm AdmiringWorm deleted the benchmark branch November 8, 2024 09:05
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.

Provide better information through the user agent
5 participants