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

Adding support to emit azmon logs in iso time format for Dedicated sku based on a flag. #10684

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Adding support to emit azmon logs in iso time format for Dedicated sku based on a flag. #10684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 16 additions & 1 deletion src/WebJobs.Script.WebHost/Diagnostics/LinuxAppServiceEventGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,13 +110,28 @@ private void WriteEvent(string eventPayload)

public override void LogAzureMonitorDiagnosticLogEvent(LogLevel level, string resourceId, string operationName, string category, string regionName, string properties)
{
_writeEvent($"{ScriptConstants.LinuxAzureMonitorEventStreamName} {(int)ToEventLevel(level)},{resourceId},{operationName},{category},{regionName},{NormalizeString(properties.Replace("'", string.Empty))},{DateTime.UtcNow}");
var timeStr = IsAzureMonitorTimeIsoFormatEnabled() ? DateTime.UtcNow.ToString("s") : DateTime.UtcNow.ToString();
_writeEvent($"{ScriptConstants.LinuxAzureMonitorEventStreamName} {(int)ToEventLevel(level)},{resourceId},{operationName},{category},{regionName},{NormalizeString(properties.Replace("'", string.Empty))},{timeStr}");
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 is applicable, but you can supply formats directly in the interpolated string:

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/tokens/interpolated

}

public static void LogUnhandledException(Exception e)
{
// Pipe the unhandled exception to stdout as part of docker logs.
Console.WriteLine($"Unhandled exception on {DateTime.UtcNow}: {e?.ToString()}");
}

private bool IsAzureMonitorTimeIsoFormatEnabled()
{
string enabledString = Environment.GetEnvironmentVariable(EnvironmentSettingNames.AzureMonitorTimeIsoFormatEnabled);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use IOptions pattern and not reading env variables directly.

Copy link
Member

Choose a reason for hiding this comment

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

@vivekjilla - here is an example -

azure-functions-host/src/WebJobs.Script.WebHost/Diagnostics/LinuxAppServiceEventGenerator.cs

Line 89 in ba3e9ef

bool detailedExecutionEventsDisabled = _functionsHostingConfigOptions.Value.DisableLinuxAppServiceExecutionDetails;

Copy link
Author

@vivekjilla vivekjilla Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I see we have IOptions<FunctionsHostingConfigOptions> which seems to read the config from FunctionsHostingConfig section. Have few questions here:

  1. Can customers set this value in FunctionsHostingConfig, and if so, how? or otherwise
  2. I believe the app settings set by the customers can be accessed via environment variables. Do we have any example of IOptions which gets populated based on environment variables.

Asking because, I think we want this to be configurable by end users, as it can potentially break things.

Or if I'm completely off the direction suggested here, I can catch up with @jviau offline.

Copy link
Contributor

Choose a reason for hiding this comment

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

@vivekjilla, IOptions pattern reads from IConfiguration (and other custom ways). And IConfiguration is built from many sources - including environment variables. So, app settings (which are env vars at runtime) are available in IOptions and IConfiguration pattern.

As for which options object to use - you can see if there is a fitting one, or you can always add your own. So, you don't have to specifically use the any existing IOptions object if they are not fitting and instead create a new one.

That is the purpose of options pattern: use an options POCO which is specific to the usage of the values and abstract away how the values are populated (in the options pipeline).

if (bool.TryParse(enabledString, out bool result))
{
return result;
}
if (int.TryParse(enabledString, out int enabledInt))
{
return Convert.ToBoolean(enabledInt);
}
return false;
}
}
}
1 change: 1 addition & 0 deletions src/WebJobs.Script/Environment/EnvironmentSettingNames.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ public static class EnvironmentSettingNames
public const string HealthPingEnabled = "WEBSITE_FUNCTIONS_HEALTH_PING_ENABLED";
public const string TestDataCapEnabled = "WEBSITE_FUNCTIONS_TESTDATA_CAP_ENABLED";
public const string AzureMonitorCategories = "WEBSITE_FUNCTIONS_AZUREMONITOR_CATEGORIES";
public const string AzureMonitorTimeIsoFormatEnabled = "FUNCTIONS_AZUREMONITOR_TIME_ISOFORMAT_ENABLED";
Copy link
Contributor

Choose a reason for hiding this comment

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

Couple issues with the env name here:

  1. This only impacts linux, but the name does not reflect that
  2. The class that consumes this appears more generic then "azure monitor". Is it accurate to include that naming here, or should the name also be more generic? Or does this class directly correlate to AzureMonitor and no other telemetry technology?

Copy link
Member

Choose a reason for hiding this comment

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

This only impacts linux, but the name does not reflect that

good point! fix applies to Linux Dedicated and EP SKUs only -WEBSITE_FUNCTIONS_AZUREMONITOR_LINUX_APPSERVICE_TIME_ISOFORMAT_ENABLED ?

The class that consumes this appears more generic then "azure monitor". Is it accurate to include that naming here, or should the name also be more generic? Or does this class directly correlate to AzureMonitor and no other telemetry technology?

This would apply to AZUREMONITOR only; @vivekjilla - do confirm by looking at Kusto logs if we see any difference in FunctionsLogs

Copy link
Author

@vivekjilla vivekjilla Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, we're using this only in the LogAzureMonitorDiagnosticLogEvent method (in that class) which is used to generate azmon logs. Verified the kusto logs and FunctionsLogs are not impacted by this change.

Was just wondering if the long names can cause any size limits etc, but otherwise I can go with this as Pragna suggested: WEBSITE_FUNCTIONS_AZUREMONITOR_LINUX_APPSERVICE_TIME_ISOFORMAT_ENABLED

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that name is quite long. Reading the issue more I think I am less worried about confusion: ISO format is already enabled on other skus/platforms, so having a customer set this to "true" for those other skus won't be concerning. I am not too concerned about a customer setting this to "false" and we still emit ISO.

This brings up another question: I assume we are only going with the config value here to avoid a breaking change? But ideally this would be the default format?

public const string FunctionsRequestBodySizeLimit = "FUNCTIONS_REQUEST_BODY_SIZE_LIMIT";
public const string FunctionsHostIdCheckLevel = "FUNCTIONS_HOSTID_CHECK_LEVEL";
public const string FunctionsPlatformConfigFilePath = "FUNCTIONS_PLATFORM_CONFIG_FILE_PATH";
Expand Down
Loading