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

[manikantanallagatla]

Issue describing the changes in this PR

For a few SKUs in functions, we want to filter azure monitor logs based on the categories subscribed by the user. This PR checks for the env variable for the categories and skips logging.

Pull request checklist

IMPORTANT: Currently, changes must be backported to the in-proc branch to be included in Core Tools and non-Flex deployments.

• Backporting to the in-proc branch is not required
  • Otherwise: Link to backporting PR
• My changes do not require documentation changes
  • Otherwise: Documentation issue linked to PR
• My changes should not be added to the release notes for the next release
  • Otherwise: I've added my notes to release_notes.md
• My changes do not need to be backported to a previous version
  • Otherwise: Backport tracked by issue/PR #issue_or_pr
• My changes do not require diagnostic events changes
  • Otherwise: I have added/updated all related diagnostic events and their documentation (Documentation issue linked to PR)
• I have added all required tests (Unit tests, E2E tests)

Additional information

Additional PR information

[jviau]

Can you give an example of these categories? Are they regular ILogger categories?

If so, we should use built in filtering with Microsoft.Extensions.Logging. We can either transform configuration, or we can simply call:

Part 1: log level filter

// condition on if this is the right sku
services.AddOptions<LoggerFilterOptions>()
    .Configure<IConfiguration>((options, config) =>
    {
        string setting = config[EnvironmentSettingNames.AzureMonitorCategories];
        options.AddFilter<AzureMonitorDiagnosticLoggerProvider>((category, level) =>
        {
            // return false when AzureMonitorCategories does not contain FunctionAppLogs
        });
    });

Part 2: IsEnabled update

// AzureMonitorDiagnosticLogger.cs
// Update the below method to return false when AzureMonitorCategories does not contain FunctionAppLogs
public bool IsEnabled(LogLevel logLevel)
{
    // 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();
}

Although I have to ask - who sets EnvironmentSettingNames.AzureMonitorCategories? Is it from the platform, or does the user explicitly set it?

[RohitRanjanMS]

I agree with @jviau that providing more context about the scenario would be beneficial.
A simple list of categories alone might not be enough; the log level will also be necessary.
The environment variable is applicable only to LinuxContainerEventGenerator and does not extend to AzureMonitor/Diagnostic Settings integrations for other skus.
If these are specified by the customer, can the existing logging configuration in host.json be leveraged instead?

[manikantanallagatla]

Can you give an example of these categories? Are they regular ILogger categories?

If so, we should use built in filtering with Microsoft.Extensions.Logging. We can either transform configuration, or we can simply call:

Part 1: log level filter

// condition on if this is the right sku
services.AddOptions<LoggerFilterOptions>()
    .Configure<IConfiguration>((options, config) =>
    {
        string setting = config[EnvironmentSettingNames.AzureMonitorCategories];
        options.AddFilter<AzureMonitorDiagnosticLoggerProvider>((category, level) =>
        {
            // return false when AzureMonitorCategories does not contain FunctionAppLogs
        });
    });

Part 2: IsEnabled update

// AzureMonitorDiagnosticLogger.cs
// Update the below method to return false when AzureMonitorCategories does not contain FunctionAppLogs
public bool IsEnabled(LogLevel logLevel)
{
    // 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();
}

Although I have to ask - who sets EnvironmentSettingNames.AzureMonitorCategories? Is it from the platform, or does the user explicitly set it?

I have removed all the changes. We already have category filter in the host code. I just had to called it in isenabled class

[jviau]

@manikantanallagatla I believe you will need to also add the filter callback I suggested. It may seem redundant but they have different purposes:

1. filter: avoid running templating on the log message
2. IsEnabled: give Logger users a hook to wrap their log statements in incase they have pre-templating code they want to skip.

I also am wondering about the IsAzureMonitorEnabled implementation as is being used in this new code paths. These are hot-paths, we need to make sure they are as performant as possible. IsAzureMonitorEnabled will be allocating an array and perform string comparison on each run. Can we optimize that? Cache the results of it somewhere?

Also this change will affect all skus - is the behavior of that env variable consistent across all skus?

[manikantanallagatla]

Can you give an example of these categories? Are they regular ILogger categories?

If so, we should use built in filtering with Microsoft.Extensions.Logging. We can either transform configuration, or we can simply call:

Part 1: log level filter

// condition on if this is the right sku
services.AddOptions<LoggerFilterOptions>()
    .Configure<IConfiguration>((options, config) =>
    {
        string setting = config[EnvironmentSettingNames.AzureMonitorCategories];
        options.AddFilter<AzureMonitorDiagnosticLoggerProvider>((category, level) =>
        {
            // return false when AzureMonitorCategories does not contain FunctionAppLogs
        });
    });

Part 2: IsEnabled update

// AzureMonitorDiagnosticLogger.cs
// Update the below method to return false when AzureMonitorCategories does not contain FunctionAppLogs
public bool IsEnabled(LogLevel logLevel)
{
    // 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();
}

Although I have to ask - who sets EnvironmentSettingNames.AzureMonitorCategories? Is it from the platform, or does the user explicitly set it?

Addressed it

[manikantanallagatla]

@manikantanallagatla I believe you will need to also add the filter callback I suggested. It may seem redundant but they have different purposes:

1. filter: avoid running templating on the log message
2. IsEnabled: give Logger users a hook to wrap their log statements in incase they have pre-templating code they want to skip.

I also am wondering about the IsAzureMonitorEnabled implementation as is being used in this new code paths. These are hot-paths, we need to make sure they are as performant as possible. IsAzureMonitorEnabled will be allocating an array and perform string comparison on each run. Can we optimize that? Cache the results of it somewhere?

Also this change will affect all skus - is the behavior of that env variable consistent across all skus?

Addressed it. Made changes to happen only for legion based SKUs

[jviau]

Couple suggestions to embrace the options pattern a bit more.

[manikantanallagatla]

Addressed all comments and updates unit tests as well

[jviau]

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?

[manikantanallagatla]

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.

[jviau]

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.

[manikantanallagatla]

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.

[RohitRanjanMS]

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?

[manikantanallagatla]

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

[jviau]

@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?

[RohitRanjanMS]

I’m concerned about the number of environment variable lookups in IsEnabled, especially since they occur for every log statement. I’ll run a benchmark on this branch to assess the impact.

AzureMonitorDiagnosticLoggerProvider is added in WebScriptHostBuilderExtension and isn’t reinitialized during specialization. This is why AppInsights is configured in ScriptHostBuilderExtensions, ensuring it’s reconfigured when specialization happens (or when host.json is modified).

Here’s what I am thinking:

• Register AzureMonitorDiagnosticLoggerProvider in ScriptHost if categories are set. The deferred logger should forward all logs from the root logger to AzureMonitorDiagnosticLogger. If categories aren’t set, AzureMonitorDiagnosticLoggerProvider won’t be registered, and the deferred logger won’t forward logs to it. Deferred logger only forwards Error and above so this option will require a change in deferred logger as well.
• Determine and cache IsAzureMonitorLoggingEnabled when ScriptHost is built, using it in IsEnabled instead of relying on environment variable lookups.

[RohitRanjanMS]

We should also disable AzureMonitorDiagnosticLoggerProvider if categories are not set.

[RohitRanjanMS]

I spoke with @jviau, and here’s our proposed approach:

• Utilize OptionsMonitor to perform the environment variable lookup just once.
• When building ScriptHost, add logic to disable the logger provider if AzureMonitorDiagnosticLogger isn’t needed. Instead of AzureMonitorDiagnosticLogger, return NullLogger.Instance.
  return _isEnabled ? new AzureMonitorDiagnosticLogger(categoryName, _hostInstanceId, _eventGenerator, _environment, _scopeProvider, _hostNameProvider, _appServiceOptions) : NullLogger.Instance;
• Can this be applied across all SKUs? This would be a solid improvement.

[manikantanallagatla]

Thanks for checking with Jacob.

Will try the first two things.

Regarding Can this be applied across all SKUs? -> We can do it if we have confidence on the change blast radius. As it is logging change which will have direct customer impact in case it has issues. Can we do for legion SKUs for now and add a task to remove legion skus check after deployments and change is working fine? That way we will reduce impact in case of issues.

CC: @vivekjilla

[RohitRanjanMS]

Sure, we can do this later.