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.

Sign up for GitHub

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

Jump to bottom

Option to disable OmexLogger #611

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

Option to disable OmexLogger #611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
11bdfae
Update
Jul 15, 2023
a49b7ab
Merge remote-tracking branch 'origin/main'
Aug 2, 2023
d6aa6bd
Merge branch 'main' of https://github.com/microsoft/Omex
Aug 22, 2023
15cc307
Merge branch 'main' of https://github.com/microsoft/Omex
Aug 25, 2023
5fed5bc
Merge remote-tracking branch 'origin/main'
Sep 19, 2023
37e68df
commit 1
Sep 20, 2023
e1240f9
Add Unit test
Sep 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/Hosting.Services/HostBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public static IHost BuildStatefulService(
/// <summary>
/// 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@AndreyTretyak AndreyTretyak Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

where TContext : ServiceContext
{
bool isStatefulService = typeof(StatefulServiceContext).IsAssignableFrom(typeof(TContext));
Expand All @@ -60,7 +60,7 @@ public static IServiceCollection AddOmexServiceFabricDependencies<TContext>(this

collection.TryAddSingleton<IServiceContext, OmexServiceFabricContext>();
collection.TryAddSingleton<IExecutionContext, ServiceFabricExecutionContext>();
return collection.AddOmexServices();
return collection.AddOmexServices(hostBuilderContext);
}

private static IHost BuildServiceFabricService<TRunner, TService, TContext>(
Expand Down Expand Up @@ -92,7 +92,7 @@ private static IHost BuildServiceFabricService<TRunner, TService, TContext>(
{
options.ServiceTypeName = serviceName;
})
.AddOmexServiceFabricDependencies<TContext>()
.AddOmexServiceFabricDependencies<TContext>(context)
.AddSingleton<IOmexServiceRegistrator, TRunner>()
.AddHostedService<OmexHostedService>();
})
Expand Down
6 changes: 3 additions & 3 deletions src/Hosting/ServiceCollectionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@ public static class ServiceCollectionExtensions
/// </summary>
public static IHostBuilder AddOmexServices(this IHostBuilder builder) =>
builder
.ConfigureServices((context, collection) => collection.AddOmexServices());
.ConfigureServices((context, collection) => collection.AddOmexServices(context));

/// <summary>
/// Add Omex Logging and ActivitySource dependencies
/// </summary>
public static IServiceCollection AddOmexServices(this IServiceCollection collection) =>
public static IServiceCollection AddOmexServices(this IServiceCollection collection, HostBuilderContext? hostBuilderContext) =>
collection
.AddOmexLogging()
.AddOmexLogging(hostBuilderContext)
.AddOmexActivitySource();

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion src/Logging/InitializationLogger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ private static ILoggingBuilder LoadInitializationLogger(this ILoggingBuilder bui
builder.AddConsole();
}

builder.AddOmexLogging();
builder.AddOmexLogging(null!); // Because this method cannot access HostBuilder's configuration.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this change is breaking legacy logging for service initialization, it doesn't meet its own goal.

return builder;
}

Expand Down
21 changes: 16 additions & 5 deletions src/Logging/ServiceCollectionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT license.

using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.DependencyInjection.Extensions;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;
using Microsoft.Omex.Extensions.Abstractions.Activities.Processing;
using Microsoft.Omex.Extensions.Abstractions.ExecutionContext;
Expand All @@ -29,23 +31,32 @@ public static IServiceCollection AddOmexServiceContext<TServiceContext>(this ISe
/// Adds Omex event logger to the factory
/// </summary>
/// <param name="builder">The extension method argument</param>
public static ILoggingBuilder AddOmexLogging(this ILoggingBuilder builder)
/// <param name="context">HostBuilderContext to access Configuration</param>
public static ILoggingBuilder AddOmexLogging(this ILoggingBuilder builder, HostBuilderContext? context)
{
builder.Services.AddOmexLogging();
builder.Services.AddOmexLogging(context);
return builder;
}

/// <summary>
/// Adds Omex event logger to the factory
/// </summary>
/// <param name="serviceCollection">The extension method argument</param>
/// <param name="context">HostBuilderContext to access Configuration</param>
/// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained</returns>
public static IServiceCollection AddOmexLogging(this IServiceCollection serviceCollection)
public static IServiceCollection AddOmexLogging(this IServiceCollection serviceCollection, HostBuilderContext? context)
{
serviceCollection.AddLogging();

serviceCollection.TryAddTransient<IServiceContext, EmptyServiceContext>();
serviceCollection.TryAddTransient<IExecutionContext, BaseExecutionContext>();
serviceCollection.AddLogging();

const string settingName = "Monitoring:OmexLoggingEnabled";
bool isEventSourceLoggingEnabled = bool.Parse(context?.Configuration.GetSection(settingName).Get<string>() ?? "true");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this works exactly like this, but I would expect something simular to this code should be possible, could you please check?

Suggested change
bool isEventSourceLoggingEnabled = bool.Parse(context?.Configuration.GetSection(settingName).Get<string>() ?? "true");
bool isEventSourceLoggingEnabled = context?.Configuration.GetSection(settingName).Get<bool>(true);

if (!isEventSourceLoggingEnabled)
{
return serviceCollection;
}

serviceCollection.TryAddTransient<IExternalScopeProvider, LoggerExternalScopeProvider>();

serviceCollection.TryAddSingleton(_ => OmexLogEventSource.Instance);
Expand Down
2 changes: 1 addition & 1 deletion tests/Hosting.Services.UnitTests/HostBuilderExtensionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public void AddOmexServiceFabricDependencies_TypesRegistered(Type typeToResolver
void CheckTypeRegistration<TContext>() where TContext : ServiceContext
{
object? obj = new ServiceCollection()
.AddOmexServiceFabricDependencies<TContext>()
.AddOmexServiceFabricDependencies<TContext>(null!)
.AddSingleton(new Mock<IHostEnvironment>().Object)
.BuildServiceProvider()
.GetService(typeToResolver);
Expand Down
2 changes: 1 addition & 1 deletion tests/Hosting.UnitTests/HostBuilderExtensionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public void AddOmexServices_TypesRegistered(Type type)
object? collectionObj = new ServiceCollection()
.AddSingleton<IConfiguration>(new ConfigurationBuilder().Build()) // Added IConfiguration because one of the dependency depends on IOptions which in turn depends on IConfiguration
.AddSingleton<IHostEnvironment>(new HostingEnvironment())
.AddOmexServices()
.AddOmexServices(null!)
.BuildServiceProvider(new ServiceProviderOptions
{
ValidateOnBuild = true,
Expand Down
2 changes: 1 addition & 1 deletion tests/Logging.UnitTests/Scrubbing/ServiceCollectionExtensionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ public void AddRegexLogScrubbingRule_Scrubs(string input, string expected)
private static ILogScrubbingRule[] GetTypeRegistrations(IServiceCollection collection)
{
IEnumerable<ILogScrubbingRule> objects = collection
.AddOmexLogging()
.AddOmexLogging(null!)
.BuildServiceProvider(new ServiceProviderOptions
{
ValidateOnBuild = true,
Expand Down
86 changes: 82 additions & 4 deletions tests/Logging.UnitTests/ServiceCollectionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,12 @@
// Licensed under the MIT license.

using System;
using System.Collections.Generic;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;
using Microsoft.Omex.Extensions.Logging.Scrubbing;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace Microsoft.Omex.Extensions.Logging.UnitTests
Expand All @@ -25,7 +29,7 @@ public void AddOmexServiceContext_OverridesContextType()
{
IServiceCollection collection = new ServiceCollection()
.AddOmexServiceContext<MockServiceContext>()
.AddOmexLogging();
.AddOmexLogging(null!);

IServiceContext context = ValidateTypeRegistration<IServiceContext>(collection);

Expand All @@ -37,22 +41,22 @@ public void AddOmexServiceContext_OverridesContextType()
[TestMethod]
public void AddOmexLoggerOnServiceCollection_RegistersLogger()
{
IServiceCollection collection = new ServiceCollection().AddOmexLogging();
IServiceCollection collection = new ServiceCollection().AddOmexLogging(null!);
ValidateTypeRegistration<ILogger<ServiceCollectionTests>>(collection);
}

[TestMethod]
public void AddOmexLoggerOnLogBuilder_RegistersLogger()
{
ILoggingBuilder builder = new MockLoggingBuilder().AddOmexLogging();
ILoggingBuilder builder = new MockLoggingBuilder().AddOmexLogging(null!);
ValidateTypeRegistration<ILogger<ServiceCollectionTests>>(builder.Services);
}

private T ValidateTypeRegistration<T>(IServiceCollection collection)
where T : class
{
T obj = collection
.AddOmexLogging()
.AddOmexLogging(null!)
.BuildServiceProvider(new ServiceProviderOptions
{
ValidateOnBuild = true,
Expand All @@ -64,6 +68,80 @@ private T ValidateTypeRegistration<T>(IServiceCollection collection)
return obj;
}

[TestMethod]
public void AddOmexLoggerOnLogBuilder_ContainHostBuilder_SettingOmexLoggingEnabled_False_OmexLoggerNotRegistered()
{
HostBuilderContext hostBuilderContext = new(new Dictionary<object, object>());
IConfigurationRoot configuration = new ConfigurationBuilder()
.AddInMemoryCollection(new Dictionary<string, string?>
{
{ "Monitoring:OmexLoggingEnabled", "false"},
})
.Build();

hostBuilderContext.Configuration = configuration;
IServiceCollection collection = new ServiceCollection()
.AddOmexServiceContext<MockServiceContext>()
.AddOmexLogging(hostBuilderContext);

Assert.ThrowsException<InvalidOperationException>(() =>
{
OmexLogEventSource logEventSource = collection
.BuildServiceProvider(new ServiceProviderOptions { ValidateOnBuild = true, ValidateScopes = true })
.GetRequiredService<OmexLogEventSource>();
});

Assert.ThrowsException<InvalidOperationException>(() =>
{
ILogEventSender eventSender = collection
.BuildServiceProvider(new ServiceProviderOptions { ValidateOnBuild = true, ValidateScopes = true })
.GetRequiredService<ILogEventSender>();
});

Assert.ThrowsException<InvalidOperationException>(() =>
{
ILoggerProvider omexLoggerProvider = collection
.BuildServiceProvider(new ServiceProviderOptions { ValidateOnBuild = true, ValidateScopes = true })
.GetRequiredService<ILoggerProvider>();
});
}

[TestMethod]
[DataTestMethod]
[DataRow(true)]
[DataRow(null!)]
public void AddOmexLoggerOnLogBuilder_ContainHostBuilder_SettingOmexLoggingEnabled_TrueOrMissing_OmexLoggerRegistered(bool? isEnabled)
{
HostBuilderContext hostBuilderContext = new(new Dictionary<object, object>());
IConfigurationRoot configuration = new ConfigurationBuilder()
.AddInMemoryCollection(isEnabled == null ? new Dictionary<string, string?>() : new Dictionary<string, string?>
{
{ "Monitoring:OmexLoggingEnabled", isEnabled.ToString()},
})
.Build();

hostBuilderContext.Configuration = configuration;
IServiceCollection collection = new ServiceCollection()
.AddOmexServiceContext<MockServiceContext>()
.AddOmexLogging(hostBuilderContext);

OmexLogEventSource logEventSource = collection
.BuildServiceProvider(new ServiceProviderOptions { ValidateOnBuild = true, ValidateScopes = true })
.GetRequiredService<OmexLogEventSource>();

ILogEventSender eventSender = collection
.BuildServiceProvider(new ServiceProviderOptions { ValidateOnBuild = true, ValidateScopes = true })
.GetRequiredService<ILogEventSender>();

ILoggerProvider omexLoggerProvider = collection
.BuildServiceProvider(new ServiceProviderOptions { ValidateOnBuild = true, ValidateScopes = true })
.GetRequiredService<ILoggerProvider>();

Assert.IsInstanceOfType<OmexLogEventSource>(logEventSource);
Assert.IsInstanceOfType<ILogEventSender>(eventSender);
Assert.IsInstanceOfType<OmexLoggerProvider>(omexLoggerProvider);
}

private class MockLoggingBuilder : ILoggingBuilder
{
public IServiceCollection Services { get; } = new ServiceCollection();
Expand Down