You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Currently, the LoggerMessage source generator requires EventId to be hardcoded in the attribute at compile time. I agree that this approach is effective for many scenarios but doesn't support cases where EventId needs to be determined dynamically. I created a library that generates LoggerMessage methods on the fly based on the LoggerExtensions.Log* invocations (example here) and users can provide EventId as a non-constant parameter. However, the current implementation of the source generator ignores this parameter and instead generates it based on the hash value.
This is not a significant issue in most cases as usually EventId is configured globally and not passed in every call, but still, there are a few scenarios where it's applicable. Another example is presented in this discussion on a related topic, where EventId can be treated as a contract where you can version your EventId parameter based on some conditions.
The existing implementation of the source generator already supports dynamic log levels, I propose that it should behave in a similar way for EventId, as illustrated below:
flowchart TD
Start[LoggerMessage] --> IsEventIdParameter{Is EventId provided as a method parameter?}
subgraph "New logic"
IsEventIdParameter --> |Yes| HasProvidedAttributeValues{Is EventId or EventName provided in the attribute?}:::newChange
HasProvidedAttributeValues --> |No| UseEventId[Use provided event id parameter]:::newChange
end
subgraph "Existing logic"
IsEventIdParameter --> |No| IsEventIdProvided
HasProvidedAttributeValues --> |Yes| IsEventIdProvided{Is EventId provided in the attribute?}
IsEventIdProvided --> |Yes| UseProvidedEventId[Use provided event id parameter]
UseProvidedEventId --> IsEventNameProvided{Is EventName provided in the attribute?}
IsEventIdProvided --> |No| CalcHashFromEventName[Set event id as hash of event/method name]
CalcHashFromEventName -->
IsEventNameProvided --> |Yes| UseProvidedEventName[Use provided event name parameter]
IsEventNameProvided --> |No| UseMethodName[Set event name as method name]
end
Loading
The change itself is quite small and simple, so I already did it here (excluding docs) for your reference, but according to the contributing rules, the issue must be approved before PR, so I believe it's better to link it here
API Proposal
namespaceMicrosoft.Extensions.Logging;publicpartialstaticclassLogger{[LoggerMessage(Level=LogLevel.Information,Message="Event has been processed in {Time} ms")]publicvoidLogEvent(EventIdeventId,doubletime);}
It has to generate the following code:
privatereadonlystruct__LogEventStruct:IReadOnlyList<KeyValuePair<string,object?>>{privatereadonlyglobal::System.Double_time;publicoverridestringToString(){varTime=this._time;return$"Event has been processed in {Time} ms";}publicstaticreadonlyglobal::System.Func<__LogEventStruct,global::System.Exception?,string>Format=(state,ex)=>state.ToString();
...}publicstaticpartialvoidLogEvent(ILoggerlogger,EventIdeventId/* <-- this parameter */,System.Doubletime){if(logger.IsEnabled(Microsoft.Extensions.Logging.LogLevel.Information)){logger.Log(Microsoft.Extensions.Logging.LogLevel.Information,eventId,// <-- has to be passed herenew__LogEventStruct(time),null,__LogEventStruct.Format);}}
API Usage
ILoggerlogger;Logger.LogEvent(logger,newEventId(42,"SomeEvent"),43);publicpartialstaticclassLogger{[LoggerMessage(Level=LogLevel.Information,Message="Event has been processed in {Time} ms")]publicvoidLogEvent(EventIdeventId,doubletime);}
Alternative Designs
Given that LoggerMessage.Define is public API and it doesn't support dynamic LogLevel and EventId and the existing implementation already uses a generated structure to handle dynamic LogLevel, I believe the most consistent and effective approach is to extend this design to support dynamic EventId the same way. This would ensure coherence across the API and maintainability in the source generator's implementation
Risks
I think the risk associated with this change is quite low. Currently, if EventId is provided as a non-template parameter, it is completely ignored and overwritten with a hash value, which is unexpected behavior from my point of view. The proposed change ensures that when a value is provided, it is correctly utilized.
This change is opt-in, so existing generated code will remain fully functional. Users who don't include an eventId parameter in their logging methods will continue to benefit from the current behavior with hardcoded/generated EventIds
Background and motivation
Currently, the
LoggerMessagesource generator requiresEventIdto be hardcoded in the attribute at compile time. I agree that this approach is effective for many scenarios but doesn't support cases whereEventIdneeds to be determined dynamically. I created a library that generatesLoggerMessagemethods on the fly based on theLoggerExtensions.Log*invocations (example here) and users can provideEventIdas a non-constant parameter. However, the current implementation of the source generator ignores this parameter and instead generates it based on the hash value.This is not a significant issue in most cases as usually
EventIdis configured globally and not passed in every call, but still, there are a few scenarios where it's applicable. Another example is presented in this discussion on a related topic, whereEventIdcan be treated as a contract where you can version yourEventIdparameter based on some conditions.The existing implementation of the source generator already supports dynamic log levels, I propose that it should behave in a similar way for
EventId, as illustrated below:flowchart TD Start[LoggerMessage] --> IsEventIdParameter{Is EventId provided as a method parameter?} subgraph "New logic" IsEventIdParameter --> |Yes| HasProvidedAttributeValues{Is EventId or EventName provided in the attribute?}:::newChange HasProvidedAttributeValues --> |No| UseEventId[Use provided event id parameter]:::newChange end subgraph "Existing logic" IsEventIdParameter --> |No| IsEventIdProvided HasProvidedAttributeValues --> |Yes| IsEventIdProvided{Is EventId provided in the attribute?} IsEventIdProvided --> |Yes| UseProvidedEventId[Use provided event id parameter] UseProvidedEventId --> IsEventNameProvided{Is EventName provided in the attribute?} IsEventIdProvided --> |No| CalcHashFromEventName[Set event id as hash of event/method name] CalcHashFromEventName --> IsEventNameProvided --> |Yes| UseProvidedEventName[Use provided event name parameter] IsEventNameProvided --> |No| UseMethodName[Set event name as method name] endThe change itself is quite small and simple, so I already did it here (excluding docs) for your reference, but according to the contributing rules, the issue must be approved before PR, so I believe it's better to link it here
API Proposal
It has to generate the following code:
API Usage
Alternative Designs
Given that
LoggerMessage.Defineis public API and it doesn't support dynamicLogLevelandEventIdand the existing implementation already uses a generated structure to handle dynamicLogLevel, I believe the most consistent and effective approach is to extend this design to support dynamicEventIdthe same way. This would ensure coherence across the API and maintainability in the source generator's implementationRisks
I think the risk associated with this change is quite low. Currently, if
EventIdis provided as a non-template parameter, it is completely ignored and overwritten with a hash value, which is unexpected behavior from my point of view. The proposed change ensures that when a value is provided, it is correctly utilized.This change is opt-in, so existing generated code will remain fully functional. Users who don't include an eventId parameter in their logging methods will continue to benefit from the current behavior with hardcoded/generated
EventIds