-
-
Notifications
You must be signed in to change notification settings - Fork 142
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
Add ExceptionlessWindowsEnvironmentInfoCollector #270
Conversation
…serObjects, and GDIObjects.
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. |
If we lazy load |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/Platforms/Exceptionless.Windows/ExceptionlessWindowsEnvironmentInfoCollector.cs
Outdated
Show resolved
Hide resolved
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @elachlan! Looks good, just a couple changes and we can get this merged.
src/Platforms/Exceptionless.Windows/ExceptionlessWindowsEnvironmentInfoCollector.cs
Outdated
Show resolved
Hide resolved
Changes are applied, let me know what you both think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thank you @elachlan
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: 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. |
Thanks for the PR! |
@niemyjski thank you for the review! Are you able to do a release sometime soon? |
We typically like to let a nightly sit for a little bit (week or two), we have a nightly NuGet feed you can use. |
Fixes #269