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

Add ExceptionlessWindowsEnvironmentInfoCollector #270

Merged
merged 7 commits into from
May 2, 2022
Merged

Add ExceptionlessWindowsEnvironmentInfoCollector #270

merged 7 commits into from
May 2, 2022

Conversation

elachlan
Copy link
Contributor

Fixes #269

@CLAassistant
Copy link

CLAassistant commented Apr 23, 2022

CLA assistant check
All committers have signed the CLA.

@elachlan
Copy link
Contributor Author

elachlan commented Apr 23, 2022

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
Copy link
Contributor Author

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

Copy link
Member

@niemyjski niemyjski left a 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.

@elachlan
Copy link
Contributor Author

elachlan commented Apr 26, 2022

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.

Copy link
Member

@ejsmith ejsmith left a 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.

@elachlan
Copy link
Contributor Author

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 elachlan requested a review from ejsmith April 28, 2022 01:10
ejsmith
ejsmith previously approved these changes Apr 28, 2022
Copy link
Member

@ejsmith ejsmith left a 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

@elachlan
Copy link
Contributor Author

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 niemyjski merged commit 5385301 into exceptionless:master May 2, 2022
@niemyjski
Copy link
Member

Thanks for the PR!

@elachlan
Copy link
Contributor Author

elachlan commented May 2, 2022

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

@niemyjski
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Add Exceptionless.Windows plugin to capture additional helpful information
4 participants