Adding support to emit azmon logs in iso time format for Dedicated sku based on a flag. #10684
Conversation
…u based on a flag.
|
|
||
| private bool IsAzureMonitorTimeIsoFormatEnabled() | ||
| { | ||
| string enabledString = Environment.GetEnvironmentVariable(EnvironmentSettingNames.AzureMonitorTimeIsoFormatEnabled); |
There was a problem hiding this comment.
Please use IOptions pattern and not reading env variables directly.
There was a problem hiding this comment.
@vivekjilla - here is an example -
There was a problem hiding this comment.
I see we have IOptions<FunctionsHostingConfigOptions> which seems to read the config from FunctionsHostingConfig section. Have few questions here:
- Can customers set this value in FunctionsHostingConfig, and if so, how? or otherwise
- 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.
There was a problem hiding this comment.
@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).
| @@ -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"; | |||
There was a problem hiding this comment.
Couple issues with the env name here:
- This only impacts linux, but the name does not reflect that
- 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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
| @@ -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}"); | |||
There was a problem hiding this comment.
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
Issue describing the changes in this PR
resolves #7864
Validations with these changes in a custom image:
First entry is without the app setting, and second entry is with the app setting enabled.
Time format changed from US to iso format.
Pull request checklist
IMPORTANT: Currently, changes must be backported to the
in-procbranch to be included in Core Tools and non-Flex deployments.in-procbranch is not requiredrelease_notes.mdAdditional information
Additional PR information