Conversation
| } | ||
|
|
||
| builder.AddOmexLogging(); | ||
| builder.AddOmexLogging(null!); // Because this method cannot access HostBuilder's configuration. |
There was a problem hiding this comment.
Given that this change is breaking legacy logging for service initialization, it doesn't meet its own goal.
| /// Registering Dependency Injection classes that will provide Service Fabric specific information for logging | ||
| /// </summary> | ||
| public static IServiceCollection AddOmexServiceFabricDependencies<TContext>(this IServiceCollection collection) | ||
| public static IServiceCollection AddOmexServiceFabricDependencies<TContext>(this IServiceCollection collection, HostBuilderContext? hostBuilderContext) |
There was a problem hiding this comment.
I don't like this approach.
It is messy.
Given that it is a breaking change for Initialization Logger, I'd be in flavor of making it a fully breaking change - updating the package version would be disabling the old logger. Since both approaches would require a code change and deployment to release, there is no release operational difference.
The main difference is that downstream consumers would need to be aware of the breaking change in the package update.
There was a problem hiding this comment.
Alternatively, define extra extension methods where one adds the new dependencies and the other adds the old dependencies, then move the configuration handling back to the service and out of the dependency registration extensions.
There was a problem hiding this comment.
I would be in faivour of doing breaking change and making AddOmexServiceFabricDependencies an extension on HostBuilderContext, since pattern of optional parameter super unusial and easy to miss for user, also HostBuilderContext contains IServiceCollection in it. We can still keep old extension method if absolutly needed but mark it as obsolete with an explanation of why change needed.
| serviceCollection.AddLogging(); | ||
|
|
||
| const string settingName = "Monitoring:OmexLoggingEnabled"; | ||
| bool isEventSourceLoggingEnabled = bool.Parse(context?.Configuration.GetSection(settingName).Get<string>() ?? "true"); |
There was a problem hiding this comment.
Not sure if this works exactly like this, but I would expect something simular to this code should be possible, could you please check?
| bool isEventSourceLoggingEnabled = bool.Parse(context?.Configuration.GetSection(settingName).Get<string>() ?? "true"); | |
| bool isEventSourceLoggingEnabled = context?.Configuration.GetSection(settingName).Get<bool>(true); |
3 participants
Context
We introduce a setting
Monitoring:OmexLoggingEnabledto allow consumers to enable or disable default OmexLogger (via Microsoft EventSource). Consumer can now register their custom logging provider, for example, OpenTelemetry or NLog.Usage
The default setting of
Monitoring:OmexLoggingEnabledisTrue.If the setting is not provided, OmexLogger will be used as default