GetSysLogIdentity() or GetSysLogId()?

Fixed

It looks like this same path creation is done repeatedly in other methods so it can be done just once in the constructor and assigned to a readonly field. Also psHomeConfigDirectory should be readonly since it is set just once in the constructor.

The entire class uses this pattern and; while i was tempted to change it; I think it's beyond the scope of this PR. I will mark the two fields as read-only, though.

Ok.

I created issue #5179 to track this.

Minor comment. You can also do:
identity.Equals(LogDefaultValueName, System.StringComparison.OrdinalIgnoreCase) which returns a Boolean.

Personal preference - I prefer having the two strings adjacent when reading the code.

Prefer consistency over personal preference for others.

Personally, I pause on Compare and not on Equals, so it is a distraction.

Well, that's two so I'll change it.

Fixed

We should be able to make this field be the base class, LogProvider, and remove these ifdefs.

Sure, that would work but I think I'll defer that to #5180

Defered

This is also defined in PropertyAccessor.cs. We should only define it once.

Will do.

Fixed - now only defined in PropertyAccessor

Instead of creating a new StringBuilder instance for each method call, I think we could just have a class property _payload instance that is cleared each time.

I'm pretty sure that will introduce race conditions.

Good point.

Not if the instance is thread local.

I suspect this is more speculation than anything else. Consider that the string builder has to be cleared and realloc'd each time plus the TLS overhead, it's likely premature to optimize without measurement. Of course, if you have reasonable evidence that object allocation will be a significant part of the overhead; I'll move it to thread local.

Garbage collection is not free - but you pay for it at random times, so it's hard to quantify.

The CLR thinks StringBuilder reuse is worthwhile, see here.

It looks like a most of these LogProvider methods have a lot of commonality with the Windows implementation (PSEtwLogProvider) and they can be implemented in the base class, with WriteEvent being specific to the derived class. I know each class is complied separately for different platforms but it would make maintaining the code easier having most of the source in one place.

This entire ETW logging stack needs to be refactored and after discussing it with Steve, I decided to defer.

Ok. Can you please create an Issue to track the work?

I've created #5180 to track this.

I'll move moved the common string building functions down into the base LogProvider class to reduce duplication.

Typo: teh should be the

Fixed

Shouldn't this field also be readonly?

Yes.

Fixed

Please add comment block for this constructor.

Fixed.

This needs to have a Marshal.FreeHGlobal() to free up the unmanaged allocation. But you can use the [MarshalAs(UnmanagedType.LPStr)] attribute on the NativeMethods.OpenLog() parameter, to get automatic marshaling.

No, this string needs to remain for the lifetime of the process. I added a comment.

I don't think this is necessary. You can initialize _resourceManager field where it is defined or in the static constructor. Also the field can be readonly.

This is the pattern used everywhere else. i believe the notion is to defer loading resources until first use.

Since Native_OpenLog takes a const char* it shouldn't need special marshaling. But I assume you do this above because it keeps a reference to the ident parameter string? If so can you update the comment to make this clear and as to why FreeHGlobal is never called (presumably because the allocation needs to exist for the life of the process).

I assume this is copied directly from Windows PowerShell manifest. I see that it has "PSWorkflow" keywords. Since we don't support workflows they can be removed at some point.

Yes, we'll need to scrub all entries that are no longer referenced.

Please do the scrub before this is merged - no sense in taking something that never should have been there in the first place.

I'll do the scrub as part of the Windows manifest work; due next.

As long as this is generated, maybe generate a switch statement instead of a dictionary.

Switch dispatch can be very fast (jump table) or a quick binary search, and the other benefit is we won't allocate a bunch of objects that might never be used.

Quoted from "https://stackoverflow.com/questions/44905/c-sharp-switch-statement-limitations-why/48259#48259"

At the end of the day, a C# switch against an integer expression on a modern system is a sub-microsecond operation, and not normally worth worrying about.

Fair enough, I'll regenerate the code to use an out parameter for the parameter count and avoid the object allocation.

Fixed.

The formatting differs significantly, please follow the style of the rest of the code base. Specifically, '(', '||', and ')' should not be on their own line.

fixed

This could go in System.Management.Automation.Utils.Separators - it helps reuse to have a common place arrays like this.

I don't see any inherit benefit in sharing this constant. It simply introduces unnecessary/unintended coupling. ON the other hand, if there's an existing pattern that this should follow; I'm happy to look at it.

It's more of a meta benefit - folks create arrays like this all over the place, sometimes in a loop.

People copy patterns, so if you follow the pattern, others will too. If you don't, they might follow your pattern - and in the new use case, it might be a bad choice.

I've left it encapsulated for now; I consider it a private implementation detail until we have a use case for sharing it.

$[EventMessage] looks like a bug.

Yes, it is.

Fixed.

Calling Contains isn't really necessary - Replace won't create a new string if there's nothing to replace, and Replace has to search the string again, so it just does more work.

And I think this whole loop would be easier to read/understand if it was something like this (basically inline the replacements since they aren't used elsewhere.)

      $message = $message -replace '%t', "`t" `
                          -replace '%n', "`n" `
                          -replace '%r', "`r" `
                          -replace '%space', ' ' `
                          -replace '%.', '.' `
                          -replace '%%', "%" 

Won't fix.

We've moved to UTF8 with no BOM in this repo.

What does that mean in practice?

It's probably fine to just use ASCII when generating this file.

Are you referring to the code or resx file? ASCII would preclude localization of the resx file; wouldn't it?

That's just a shortcut assuming that we don't have characters requiring UTF8 encoding, which we don't.

This would only preclude localization if we generated the resx file from a localized manifest. As we're not even localizing error messages, I think we're a long way off from localizing the event log, and when that happens, it might not depend on generating new resx files - so I wouldn't worry about that for now.

Fair enough, I'll switch to ascii.

Fixed.

Please do the scrub before this is merged - no sense in taking something that never should have been there in the first place.

This description does not describe the guid in the example.

I'll fix it.

Fixed.