exceptionless / Exceptionless.Net

Exceptionless clients for the .NET platform
https://exceptionless.com
Other
551 stars 142 forks source link

Add ExceptionlessWindowsEnvironmentInfoCollector #270

Closed elachlan closed 2 years ago

elachlan commented 2 years ago

Fixes #269

CLAassistant commented 2 years ago

CLA assistant check
All committers have signed the CLA.

elachlan commented 2 years ago

I think it might be a good idea to change the config/log fields in DefaultEnvironmentInfoCollector to be protected so that inherited classes can log warnings and check the config without holding a reference themselves. This would change the API surface though, so I'd thought I'd raise it first.

Additionally, we can use a lazy loaded static System.Diagnostics.Process field and then call its refresh() method once per GetEnvironmentInfo() call to reduce the expense per call.

elachlan commented 2 years ago

If we lazy load Process, we need to implement a dispose method as we access Process.Handle. https://stackoverflow.com/questions/16957320/what-does-process-dispose-actually-do

elachlan commented 2 years ago

Thanks for the PR, I will talk to the rest of the team about taking a dependency on win32. We do support this library on .NET 5+ even if it's windows only currently, it may not be some day. Also need to discuss if we feel like it's a good option to create new first-class properties that we would need to support everywhere. I feel like these properties probably should be in a data bag.

I'd like to hope that one-day winforms would run on platforms other than windows. I did ask Microsoft on GitHub about it. But was told winforms will forever be windows only due to its tight coupling with win32.

In WINE calls to these APIs will throw not implemented exceptions I think.

In any case, we can detect the platform and only populate the variables when it's running on windows. Handles are windows specific and so wouldn't make sense to attempt to be populated on Linux.

I'll move the properties to the databag, not a problem.

elachlan commented 2 years ago

Thanks for the contribution @elachlan! Looks good, just a couple changes and we can get this merged.

Changes are applied, let me know what you both think.

elachlan commented 2 years ago

The only nitpick I have is that GetGuiResources has the ability to query the peak count on GDI and User objects. Which might be helpful. But as per the documentation for it, it isn't supported on older windows versions: Windows Server 2008, Windows Vista, Windows Server 2003 and Windows XP: This value is not supported until Windows 7 and Windows Server 2008 R2.

That being said, I don't think that matters because of the requirement for .NET 4.6.2 by this package. Which is Win 7 Minimum.

niemyjski commented 2 years ago

Thanks for the PR!

elachlan commented 2 years ago

@niemyjski thank you for the review! Are you able to do a release sometime soon?

niemyjski commented 2 years ago

We typically like to let a nightly sit for a little bit (week or two), we have a nightly NuGet feed you can use.