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

Factor LibraryImports into Common\ for System.Diagnostics.EventLog #109196

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

deeprobin
Copy link
Contributor

Factor LibraryImports into Common\ - See #28035

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Oct 24, 2024
@AaronRobinsonMSFT
Copy link
Member

I'm not so sure this is better actually. All of these are for a very narrow scenario. I think I prefer them to be in a single file rather than spread across multiple. I realize it is against the recommendation, but these are all niche enough being in the same file just seems better.

/cc @dotnet/dotnet-diag

@noahfalk
Copy link
Member

Including @dotnet/area-system-diagnostics-eventlog

@deeprobin
Copy link
Contributor Author

I'm not so sure this is better actually. All of these are for a very narrow scenario. I think I prefer them to be in a single file rather than spread across multiple. I realize it is against the recommendation, but these are all niche enough being in the same file just seems better.

/cc @dotnet/dotnet-diag
/cc @dotnet/area-system-diagnostics-eventlog

Hm, that's a design question.
If everything were in one file here, the file would of course be very large again.

I have no idea if there are any other projects in the runtime repo that could use the wevtapi, but I think the primary consumer remains System.Diagnostics.EventLog.

Personally, I find the approach with the individual files better and more structured, but that's probably a subjective thing.


Another question is how to proceed with the EventLogHandle (internal). Are handles something that belongs in the Common\ folder or not?

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-diagnostics-eventlog
See info in area-owners.md if you want to be subscribed.

@deeprobin
Copy link
Contributor Author

Hey @ericstj, I think this is your area. Could you review the PR 😄?

@ericstj
Copy link
Member

ericstj commented Oct 29, 2024

Hey @ericstj, I think this is your area. Could you review the PR 😄?

https://github.com/orgs/dotnet/teams/area-system-diagnostics-eventlog - @ViktorHofer @carlossanlop can you chime in.

I tend to share @noahfalk's opinion here. I don't like churning all this code without a good reason. If you really needed to share this code then that might be a good reason. If not then it's just risk in a library that's legacy. We do document a higher bar for this library: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Diagnostics.EventLog/README.md#contribution-bar

@deeprobin
Copy link
Contributor Author

@ericstj
Thank you for your comments. This probably also applies to other libraries that could be labeled as “legacy” here.
I have put this in the issue - we can close it imo.

Why is System.Diagnostics.EventLog actually legacy? Even though I am a fan of ASP.NET applications on Linux instances, there are cases where applications are deployed on Windows.
Sure, you could write to a log file here, but what's wrong with a central log component from the operating system. For Linux the equivalent would be syslog I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Diagnostics.EventLog community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants