feat: Enumerate all .NET Framework installations #572

lucas-zimerman · 2020-11-04T16:46:17Z

Closes #531

When running a .NET Application, Sentry will add a list of installed .NET Frameworks with the events.
The operation can be opted-out.

codecov-io · 2020-11-04T19:34:06Z

Codecov Report

Merging #572 (71713bd) into main (2e3a433) will increase coverage by 0.15%.
The diff coverage is 81.57%.

@@            Coverage Diff             @@
##             main     #572      +/-   ##
==========================================
+ Coverage   62.88%   63.04%   +0.15%     
==========================================
  Files         176      179       +3     
  Lines        4985     5019      +34     
  Branches      854      861       +7     
==========================================
+ Hits         3135     3164      +29     
- Misses       1624     1627       +3     
- Partials      226      228       +2

Impacted Files	Coverage Δ
...rmAbstractions/NetFxInstallationsEventProcessor.cs	`72.72% <72.72%> (ø)`
src/Sentry/SentryOptionsExtensions.cs	`68.62% <75.00%> (+0.54%)`	⬆️
...ntry/Integrations/NetFxInstallationsIntegration.cs	`100.00% <100.00%> (ø)`
...Sentry/PlatformAbstractions/FrameworkInfo.NetFx.cs	`71.23% <100.00%> (ø)`
...ormAbstractions/FrameworkInstallationExtensions.cs	`100.00% <100.00%> (ø)`
src/Sentry/SentryOptions.cs	`93.10% <100.00%> (+0.18%)`	⬆️
...ntry/PlatformAbstractions/FrameworkInstallation.cs	`50.00% <0.00%> (+25.00%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e3a433...d28b296. Read the comment docs.

bruno-garcia

Looking nice! Got a screenshot of what it looks like in Sentry?

CHANGELOG.md

bruno-garcia · 2020-11-05T01:19:15Z

src/Sentry/Internal/NetFxInstallationsEventProcessor.cs

+        internal static readonly string NetFxInstallationsKey = ".NET Framework";
+
+        private readonly Lazy<IEnumerable<FrameworkInstallation>> _netFxInstallations =
+            new Lazy<IEnumerable<FrameworkInstallation>>(() => FrameworkInfo.GetInstallations());


You need to consider multiple threads will capture events concurrently.

Use ExecutionAndPublication here.

Another point is error handling. You need to make sure an exception thrown in Lazy ctor doesn't crash the whole process (or in this case since we'd handle it in the SDK, drop the whole event).

As stated earlier, a Lazy<T> object always returns the same object or value that it was initialized with, and therefore the Value property is read-only. If you enable exception caching, this immutability also extends to exception behavior. If a lazy-initialized object has exception caching enabled and throws an exception from its initialization method when the Value property is first accessed, that same exception is thrown on every subsequent attempt to access the Value property

We need to enable such caching, and when trying to access Value do that under a try catch.
If an exception is thrown, log that with options.DiagnosticLogger?.LogError and flip a flag in the object (a field) so that you don't try to access Value again. From that point a LogDebug is enough just to note that we skipped the integration.

bruno-garcia · 2020-11-05T01:23:05Z

test/Sentry.Tests/Internals/NetFxInstallationsEventProcessorTests.cs

+        }
+
+        [Fact]
+        public void Process_NetFxInstallationsKeyExist_UnchangedSentryEvent()


We need a test that verifies we can actually add something to the context. Maybe call FrameworkInfo.GetInstallations() and assert the items are there.

Ideally another that we can serialize the contexts once you add the items to that list.

bruno-garcia · 2020-11-05T01:50:41Z

Also check if it returns anything because this code will run on Mono on Linux for example and there's no .NET Framework there

lucas-zimerman · 2020-11-05T20:30:02Z

Looking nice! Got a screenshot of what it looks like in Sentry?

Sure, for the next Commit :D

EDIT: I don't think there'll be any problems with mono due to #if NETFX (and GetInstallations is using Win32 code)

bruno-garcia · 2020-11-05T22:50:36Z

EDIT: I don't think there'll be any problems with mono due to #if NETFX (and GetInstallations is using Win32 code)

#if NETFX is true on Mono

lucas-zimerman · 2020-11-05T22:51:59Z

EDIT: I don't think there'll be any problems with mono due to #if NETFX (and GetInstallations is using Win32 code)

#if NETFX is true on Mono

Yeah, the test is failing because of that :c

src/Sentry/PlatformAbstractions/FrameworkInstallationExtensions.cs

src/Sentry/PlatformAbstractions/NetFxInstallationsEventProcessor.cs

bruno-garcia · 2020-11-06T01:00:37Z

src/Sentry/PlatformAbstractions/NetFxInstallationsEventProcessor.cs

+        {
+            var versionsDictionary = new Dictionary<string, string>();
+            var installations = FrameworkInfo.GetInstallations();
+            foreach (var profile in installations.Select(p => p.Profile).Distinct())


Why Distinct here? Isn't this going to work on the object references?

The goal of distinct is to avoid repeating keys to be inserted into the Dictionary and the foreach loop will then group the releases by profiles.

But you're doing Distinct on a reference type so unless GetHashCode and Equals was overriden, it will dedupe on the reference.

Confirmed offline that it fails with dupe key in Map otherwise

src/Sentry/PlatformAbstractions/NetFxInstallationsEventProcessor.cs

test/Sentry.Tests/Integrations/NetFxInstallationsIntegrationTests.cs

test/Sentry.Tests/PlatformAbstractions/FrameworkInstallationExtensionsTests.cs

bruno-garcia

LGTM. Just clarifying if that Distinct does something or not

src/Sentry/PlatformAbstractions/FrameworkInstallationExtensions.cs

bruno-garcia · 2020-11-06T18:50:13Z

src/Sentry/PlatformAbstractions/NetFxInstallationsEventProcessor.cs

+        {
+            var versionsDictionary = new Dictionary<string, string>();
+            var installations = FrameworkInfo.GetInstallations();
+            foreach (var profile in installations.Select(p => p.Profile).Distinct())


But you're doing Distinct on a reference type so unless GetHashCode and Equals was overriden, it will dedupe on the reference.

…s.cs

Tyrrrz · 2020-11-06T18:56:25Z

src/Sentry/PlatformAbstractions/FrameworkInfo.NetFx.cs

@@ -103,7 +103,7 @@ public static IEnumerable<FrameworkInstallation> GetInstallations()
                if (version != null && versionKey.GetInt("Install") == 1)
                {
                    // 1.0 to 3.5
-                    Version.TryParse(version, out var parsed);
+                    _ = Version.TryParse(version, out var parsed);


Is this some sort of analyzer we have that warns about this? I don't see it on my end and I'd say adding those _ makes things more noisy.

I get the warnings too. Visual Studio

The Sentry project is a bit noisy about unused return parameters, it compiles, but leaves a lot of error messages here. (I am running Visual Studio Community without any addon)

lucas-zimerman added 2 commits November 4, 2020 12:22

Added NetFxInstallations event processor.

29acf03

+Lazy

4c109bf

lucas-zimerman requested a review from bruno-garcia as a code owner November 4, 2020 16:46

lucas-zimerman marked this pull request as draft November 4, 2020 18:28

Fixes requested

8485510

Added changelog.

5773e93

bruno-garcia reviewed Nov 5, 2020

View reviewed changes

lucas-zimerman added 2 commits November 5, 2020 18:56

code cleanup and some changes requested by Bruno.

ce0dd22

bug fixes with some asserts.

2634cee

lucas-zimerman added 2 commits November 5, 2020 20:13

(to remove) test to validate the mac/ubuntu error output.

1da6fc4

Assured the NetFxEventProcessor is not added when mono is running.

2557878

lucas-zimerman marked this pull request as ready for review November 6, 2020 00:07

bruno-garcia reviewed Nov 6, 2020

View reviewed changes

Changes requested by Bruno.

71713bd

bruno-garcia approved these changes Nov 6, 2020

View reviewed changes

Update src/Sentry/PlatformAbstractions/FrameworkInstallationExtension…

d28b296

…s.cs

Tyrrrz reviewed Nov 6, 2020

View reviewed changes

bruno-garcia merged commit db6d590 into getsentry:main Nov 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Enumerate all .NET Framework installations #572

feat: Enumerate all .NET Framework installations #572

lucas-zimerman commented Nov 4, 2020 •

edited by bruno-garcia

Loading

codecov-io commented Nov 4, 2020 •

edited

Loading

bruno-garcia left a comment

bruno-garcia Nov 5, 2020

bruno-garcia Nov 5, 2020

bruno-garcia commented Nov 5, 2020

lucas-zimerman commented Nov 5, 2020 •

edited

Loading

bruno-garcia commented Nov 5, 2020

lucas-zimerman commented Nov 5, 2020

bruno-garcia Nov 6, 2020

lucas-zimerman Nov 6, 2020

bruno-garcia Nov 6, 2020

bruno-garcia Nov 6, 2020

bruno-garcia left a comment

bruno-garcia Nov 6, 2020

Tyrrrz Nov 6, 2020

bruno-garcia Nov 6, 2020

lucas-zimerman Nov 6, 2020

feat: Enumerate all .NET Framework installations #572

feat: Enumerate all .NET Framework installations #572

Conversation

lucas-zimerman commented Nov 4, 2020 • edited by bruno-garcia Loading

codecov-io commented Nov 4, 2020 • edited Loading

Codecov Report

bruno-garcia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruno-garcia commented Nov 5, 2020

lucas-zimerman commented Nov 5, 2020 • edited Loading

bruno-garcia commented Nov 5, 2020

lucas-zimerman commented Nov 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruno-garcia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucas-zimerman commented Nov 4, 2020 •

edited by bruno-garcia

Loading

codecov-io commented Nov 4, 2020 •

edited

Loading

lucas-zimerman commented Nov 5, 2020 •

edited

Loading