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

Possibility to rename hard-coded "EventId" property #200

Open
rickardp opened this issue Mar 3, 2022 · 1 comment
Open

Possibility to rename hard-coded "EventId" property #200

rickardp opened this issue Mar 3, 2022 · 1 comment

Comments

@rickardp
Copy link

rickardp commented Mar 3, 2022

If I read the code correctly in SerilogLogger, there is only one property that is being set directly by the code in this library: the EventId property. Since this is a static concept in Microsoft.Extensions.Logging, it gets translated to general SeriLog properties in this class. The problem is on the SeriLog side, this clashes with the properties provided by the application (which it does not on the Microsoft.Extensions.Logging side). As this property is hard coded here it is difficult to change the behaviour if integrating with existing logging code, or if application developers expects to be able to use this property, which would prevent adoption of the LoggerMessage-style logging.

For some background, EventId is a well-known primary key in many of our services and not being able to use this name would mean a lot of migration and confusion (resulting in high probability of logging bugs). It is not an immediate issue as we are not using the event ID/Name in these log lines currently, but since Microsoft seems to recommend migrating to LoggerMessage where performance is important this will be a future source of bugs for us that would be nice to have a solution for.

Would it be possible to introduce the option to reconfigure the name for this property, or introduce a (possibly configurable) common prefix for all "system-defined" properties? As far as I can see, "EventId" is the only property that is explicitly hard coded in this library in normal cases, but I did not look beyond the SerilogLogger class so I may be wrong here.

@nblumhardt
Copy link
Member

Thanks for the note, Rikard. This would be nice to enable, but we'd need to think about how we'd expose it through the higher-level APIs like UseSerilog().

If anyone has a chance to look into it, a proposal for introducing an overload of UseSerilog() that accepts a trailing SerilogLoggingOptions object carrying parameters like WriteToProviders and PreserveStaticLogger would set the groundwork for this kind of change.

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

No branches or pull requests

2 participants