Skip to content

Filter Azure monitor logs when customer does not subscribe for the categories #10982

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

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from
Open

Filter Azure monitor logs when customer does not subscribe for the categories #10982

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a8cd4b0
try logger changes
Apr 19, 2025
c1bc534
address all comments
Apr 20, 2025
f30df7c
spaces change
Apr 20, 2025
39b7f2c
correct filter
Apr 21, 2025
4628bbe
clean code
Apr 22, 2025
e8a0b4c
address comments
Apr 27, 2025
81e274f
Addres more comments
Apr 29, 2025
47d290a
Update tests
Apr 30, 2025
c1823c1
Fix tests build
May 2, 2025
9803480
address comments
May 13, 2025
e3059af
Address commments
May 13, 2025
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
13 changes: 7 additions & 6 deletions src/WebJobs.Script.WebHost/Diagnostics/AzureMonitorDiagnosticLogger.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public class AzureMonitorDiagnosticLogger : ILogger
private readonly IEventGenerator _eventGenerator;
private readonly IEnvironment _environment;
private readonly IExternalScopeProvider _scopeProvider;
private AppServiceOptions _appServiceOptions;
private IOptionsMonitor<AppServiceOptions> _appServiceOptionsMonitor;

public AzureMonitorDiagnosticLogger(string category, string hostInstanceId, IEventGenerator eventGenerator, IEnvironment environment, IExternalScopeProvider scopeProvider,
HostNameProvider hostNameProvider, IOptionsMonitor<AppServiceOptions> appServiceOptionsMonitor)
Expand All @@ -40,10 +40,7 @@ public AzureMonitorDiagnosticLogger(string category, string hostInstanceId, IEve
_environment = environment ?? throw new ArgumentNullException(nameof(environment));
_scopeProvider = scopeProvider ?? throw new ArgumentNullException(nameof(scopeProvider));
_hostNameProvider = hostNameProvider ?? throw new ArgumentNullException(nameof(hostNameProvider));
_ = appServiceOptionsMonitor ?? throw new ArgumentNullException(nameof(appServiceOptionsMonitor));

appServiceOptionsMonitor.OnChange(newOptions => _appServiceOptions = newOptions);
_appServiceOptions = appServiceOptionsMonitor.CurrentValue;
_appServiceOptionsMonitor = appServiceOptionsMonitor ?? throw new ArgumentNullException(nameof(appServiceOptionsMonitor));

_roleInstance = _environment.GetInstanceId();

Expand All @@ -54,6 +51,10 @@ public AzureMonitorDiagnosticLogger(string category, string hostInstanceId, IEve

public bool IsEnabled(LogLevel logLevel)
Copy link
Member

Choose a reason for hiding this comment

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

Is there any scenario where this could switch from enabled to disabled? I believe that once enabled, it will stay active. Would you like to store the state so that, after activation, all checks can be bypassed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Customer can unsubscribe the categories which they have subscribed in the past. Then, this would go from enabled to disabled.

In PR, I was initially caching the isenabled value. But, the options monitor in essence handles it for us. We are not doing too much computation like string split or something. We are checking env variables in this change. So, again caching over the options monitor is redundant

Copy link
Contributor

@jviau jviau May 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@RohitRanjanMS my understanding of the changes is that this will correctly enable/disable dynamically as the config value is changed (due to the usage of OptionsMonitor).

With that said, the primary interaction point is an app setting, which when changed will always cause a site restart.

Do you see some issue or concern with how this will enable/disable?

{
if (_environment.IsConsumptionOnLegion() && !_appServiceOptionsMonitor.CurrentValue.IsAzureMonitorLoggingEnabled)
{
return false;
}
// We want to instantiate this Logger in placeholder mode to warm it up, but do not want to log anything.
return !string.IsNullOrEmpty(_hostNameProvider.Value) && !_environment.IsPlaceholderModeEnabled();
}
Expand Down Expand Up @@ -107,7 +108,7 @@ public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Except
using (JsonTextWriter writer = new JsonTextWriter(sw) { Formatting = Formatting.None })
{
writer.WriteStartObject();
WritePropertyIfNotNull(writer, "appName", _appServiceOptions.AppName);
WritePropertyIfNotNull(writer, "appName", _appServiceOptionsMonitor.CurrentValue.AppName);
WritePropertyIfNotNull(writer, "roleInstance", _roleInstance);
WritePropertyIfNotNull(writer, "message", formattedMessage);
WritePropertyIfNotNull(writer, "category", _category);
Expand Down
13 changes: 13 additions & 0 deletions src/WebJobs.Script.WebHost/WebScriptHostBuilderExtension.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,13 @@
using Microsoft.Azure.WebJobs.Script.WebHost.Management;
using Microsoft.Azure.WebJobs.Script.WebHost.Middleware;
using Microsoft.Azure.WebJobs.Script.WebHost.Storage;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.DependencyInjection.Extensions;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using static Microsoft.Azure.WebJobs.Script.Utility;

namespace Microsoft.Azure.WebJobs.Script.WebHost
{
Expand Down Expand Up @@ -91,6 +93,17 @@ public static IHostBuilder AddWebScriptHost(this IHostBuilder builder, IServiceP
loggingBuilder.AddWebJobsSystem<SystemLoggerProvider>();
if (environment.IsAzureMonitorEnabled())
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, this is awkward. Going back and forth on this PR and it looks like it is completely unnecessary. We already completely disable AzureMonitor / shoebox by simply not registering the AzureMonitorDiagnosticLoggerProvider - and this is applicable to all skus.

Which means, is this PR even necessary anymore?

Copy link
Contributor Author

@manikantanallagatla manikantanallagatla May 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yup. I looked at this code and tested the flow. Without the PR, the logs are not getting disabled. I think what was happening is that:

  • Legion pods start in placeholder mode and this code is executed at that time. That time, we enable azure monitor.
  • When legion pods are specialized, the env variable is set(lets say to None when categories are not subscribed) and we never disable the Azure monitor as we are not doing any options monitor or env variable monitor without the PR.
  • Thats why I started with checking for this env variable in is_enabled flow in aAzureMonitorDiagnosticLogger.cs in the PR initially
  • Then we went through this options monitor flow which is more efficient.

In summary, specialization is updating the env variable and we need options monitor on that env variable in order to enable/ disable azure monitor logs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you saying AzureMonitor env variable is set during placeholder mode? I would expect it to be the opposite: no env variable set, logger not registered, specialization brings in the azure monitor variable, but it has no effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean:

During placeholder mode, WEBSITE_FUNCTIONS_AZUREMONITOR_CATEGORIES env is set to null. it is not set to anything. So, logger is initialized as we are returning true here for null case:

azure-functions-host/src/WebJobs.Script/Environment/EnvironmentExtensions.cs

Line 75 in e04ccef

return true;

It is also mentioned here that the logger is initialized but the logs are filtered out in placeholder mode:
https://github.com/Azure/azure-functions-host/blob/dev/src/WebJobs.Script.WebHost/Diagnostics/AzureMonitorDiagnosticLogger.cs#L58

So, specialization updates the env variable but it has no effect of disabling the logs.

I think we both are saying samething.

As env variable update is not taking effect, we need this PR so we can disable logging when that env variable is set to None.

{
if (environment.IsConsumptionOnLegion())
{
loggingBuilder.Services.AddOptions<LoggerFilterOptions>()
.Configure<IOptionsMonitor<AppServiceOptions>>((filters, options) =>
{
filters.AddFilter<AzureMonitorDiagnosticLoggerProvider>((category, level) =>
{
return options.CurrentValue.IsAzureMonitorLoggingEnabled;
});
});
}
loggingBuilder.Services.AddSingleton<ILoggerProvider, AzureMonitorDiagnosticLoggerProvider>();
}

Expand Down
2 changes: 2 additions & 0 deletions src/WebJobs.Script/Config/AppServiceOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,7 @@ public class AppServiceOptions
public string RuntimeSiteName { get; set; }

public string SlotName { get; set; }

public bool IsAzureMonitorLoggingEnabled { get; set; }
}
}
3 changes: 3 additions & 0 deletions src/WebJobs.Script/Config/AppServiceOptionsSetup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// Licensed under the MIT License. See License.txt in the project root for license information.

using Microsoft.Extensions.Options;
using static Microsoft.Azure.WebJobs.Script.EnvironmentSettingNames;
using static Microsoft.Azure.WebJobs.Script.Utility;

namespace Microsoft.Azure.WebJobs.Script.Configuration
{
Expand All @@ -20,6 +22,7 @@ public void Configure(AppServiceOptions options)
options.SubscriptionId = _environment.GetSubscriptionId() ?? string.Empty;
options.RuntimeSiteName = _environment.GetRuntimeSiteName() ?? string.Empty;
options.SlotName = _environment.GetSlotName() ?? string.Empty;
options.IsAzureMonitorLoggingEnabled = _environment.IsAzureMonitorEnabled();
}
}
}
11 changes: 3 additions & 8 deletions src/WebJobs.Script/Environment/EnvironmentExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
using Microsoft.Azure.WebJobs.Script.Description;
using Microsoft.Azure.WebJobs.Script.Workers.Rpc;
using static Microsoft.Azure.WebJobs.Script.EnvironmentSettingNames;
using static Microsoft.Azure.WebJobs.Script.Utility;

namespace Microsoft.Azure.WebJobs.Script
{
Expand Down Expand Up @@ -69,13 +70,7 @@ public static bool IsEasyAuthEnabled(this IEnvironment environment)
/// </summary>
public static bool IsAzureMonitorEnabled(this IEnvironment environment)
{
string value = environment.GetEnvironmentVariable(AzureMonitorCategories);
if (value == null)
{
return true;
}
string[] categories = value.Split(',');
return categories.Contains(ScriptConstants.AzureMonitorTraceCategory);
return IsAzureMonitorLoggingEnabled(environment.GetEnvironmentVariable(AzureMonitorCategories));
}

/// <summary>
Expand Down Expand Up @@ -337,7 +332,7 @@ public static bool IsLinuxConsumptionOnAtlas(this IEnvironment environment)
string.IsNullOrEmpty(environment.GetEnvironmentVariable(LegionServiceHost));
}

private static bool IsConsumptionOnLegion(this IEnvironment environment)
public static bool IsConsumptionOnLegion(this IEnvironment environment)
{
return !environment.IsAppService() &&
(!string.IsNullOrEmpty(environment.GetEnvironmentVariable(ContainerName)) ||
Expand Down
1 change: 1 addition & 0 deletions src/WebJobs.Script/ScriptConstants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ public static class ScriptConstants
public const string ExtendedPlatformChannelNameUpper = "EXTENDED";

public const string AzureMonitorTraceCategory = "FunctionAppLogs";
public const string DefaultAzureMonitorCategories = "None";

public const string KubernetesManagedAppName = "K8SE_APP_NAME";
public const string KubernetesManagedAppNamespace = "K8SE_APP_NAMESPACE";
Expand Down
16 changes: 16 additions & 0 deletions src/WebJobs.Script/Utility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1139,6 +1139,22 @@ public static bool TryReadAsBool(IDictionary<string, object> properties, string
return result = false;
}

public static bool IsAzureMonitorLoggingEnabled(string azureMonitorcategoriesSubscribed)
{
if (azureMonitorcategoriesSubscribed == null)
{
return true;
}
if (string.Equals(ScriptConstants.DefaultAzureMonitorCategories, azureMonitorcategoriesSubscribed, StringComparison.Ordinal))
{
// Default value for the env variable is None.
// This is set when customer does not subscribe any category.
return false;
}
string[] categories = azureMonitorcategoriesSubscribed.Split(',');
return categories.Contains(ScriptConstants.AzureMonitorTraceCategory);
}

private class FilteredExpandoObjectConverter : ExpandoObjectConverter
{
public override bool CanWrite => true;
Expand Down
41 changes: 18 additions & 23 deletions test/WebJobs.Script.Tests/Eventing/DiagnosticLoggerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,35 +206,30 @@ public void Log_Sanitizes()
Assert.True(JToken.DeepEquals(actual, expected), $"Actual: {actual.ToString()}{Environment.NewLine}Expected: {expected.ToString()}");
}

[Fact]
public void Log_DisabledIfPlaceholder()
[Theory]
[InlineData(EnvironmentSettingNames.AzureWebsitePlaceholderMode, "1", false, false, false)] // Placeholder
[InlineData(EnvironmentSettingNames.AzureWebsitePlaceholderMode, "1", true, false, false)] // Placeholder
[InlineData(EnvironmentSettingNames.AzureWebsiteHostName, null, false, false, false)] // NoSiteName
[InlineData(EnvironmentSettingNames.AzureWebsiteHostName, null, true, false, false)] // NoSiteName
[InlineData(EnvironmentSettingNames.AzureWebsiteHostName, "host", false, false, true)]
[InlineData(EnvironmentSettingNames.AzureWebsiteHostName, "host", true, false, false)]
[InlineData(EnvironmentSettingNames.AzureWebsiteHostName, "host", true, true, true)]
public void Log_IsEnabled(string envVariableName, string envVariableVale, bool isConsumptionOnLegion, bool isAzureMonitorEnabled, bool isDisabled)
{
string message = "TestMessage";
string functionInvocationId = Guid.NewGuid().ToString();
string activityId = Guid.NewGuid().ToString();

_environment.SetEnvironmentVariable(EnvironmentSettingNames.AzureWebsitePlaceholderMode, "1");

_logger.LogInformation(message);

Assert.False(_logger.IsEnabled(LogLevel.Information));
_mockEventGenerator.Verify(m => m.LogAzureMonitorDiagnosticLogEvent(It.IsAny<LogLevel>(), It.IsAny<string>(), It.IsAny<string>(), It.IsAny<string>(), It.IsAny<string>(), It.IsAny<string>()), Times.Never);
}

[Fact]
public void Log_DisabledIfNoSiteName()
{
string message = "TestMessage";
string functionInvocationId = Guid.NewGuid().ToString();
string activityId = Guid.NewGuid().ToString();
_environment.SetEnvironmentVariable(EnvironmentSettingNames.AzureWebsiteHostName, null);

// Recreate the logger was we cache the site name in the constructor
ILogger logger = new AzureMonitorDiagnosticLogger(_category, _hostInstanceId, _mockEventGenerator.Object, _environment, new LoggerExternalScopeProvider(), _hostNameProvider, _appServiceOptionsWrapper);
_environment.SetEnvironmentVariable(envVariableName, envVariableVale);
if (isConsumptionOnLegion)
{
_environment.SetEnvironmentVariable(EnvironmentSettingNames.AzureWebsiteInstanceId, string.Empty);
_environment.SetEnvironmentVariable(EnvironmentSettingNames.ContainerName, "containername");
_environment.SetEnvironmentVariable(EnvironmentSettingNames.LegionServiceHost, "legionhost");
}

logger.LogInformation(message);
_appServiceOptionsWrapper.CurrentValue.IsAzureMonitorLoggingEnabled = isAzureMonitorEnabled;

Assert.False(logger.IsEnabled(LogLevel.Information));
Assert.Equal(isDisabled, _logger.IsEnabled(LogLevel.Information));
_mockEventGenerator.Verify(m => m.LogAzureMonitorDiagnosticLogEvent(It.IsAny<LogLevel>(), It.IsAny<string>(), It.IsAny<string>(), It.IsAny<string>(), It.IsAny<string>(), It.IsAny<string>()), Times.Never);
}

Expand Down
2 changes: 2 additions & 0 deletions test/WebJobs.Script.Tests/Extensions/EnvironmentExtensionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ public void IsVMSS_RetrunsExpectedResult(string roleInstanceId, bool expected)
[InlineData(null, true)]
[InlineData("", false)]
[InlineData("Foo,FunctionAppLogs,Bar", true)]
[InlineData("FunctionAppLogs", true)]
[InlineData("None", false)]
[InlineData("Foo,Bar", false)]
public void IsAzureMonitorEnabled_ReturnsExpectedResult(string value, bool expected)
{
Expand Down