Linux Consumption metrics publisher for Legion #10750
Conversation
| /// <returns><see cref="true"/> if the app is V1 Linux Consumption running on Legion; otherwise, <see cref="false"/>.</returns> | ||
| public static bool IsV1LinuxConsumptionOnLegion(this IEnvironment environment) | ||
| { | ||
| return IsLinuxConsumptionOnLegion(environment); //&& environment.WebsiteSkuIsDynamic(); |
There was a problem hiding this comment.
Need to figure out how to distinguish between cv2 vs cv1 in legion based on environment variables.
There was a problem hiding this comment.
There should be a unique environmental value/flag that we're specifying for Cv1 that isn't present for Flex apps. Perhaps the SKU value? @balag0 what would you recommend we use to distinguish here? We need this check to return True for Cv1 on Legion and False for Flex apps.
There was a problem hiding this comment.
CV2
LEGION_SERVICE_HOST = 1
WEBSITE_SKU = FlexConsumption
CV1 on Legion
LEGION_SERVICE_HOST = 1
WEBSITE_SKU_NAME or WEBSITE_SKU = Dynamic
vivekjilla
changed the title
| _environment = environment ?? throw new ArgumentNullException(nameof(environment)); | ||
| _logger = logger ?? throw new ArgumentNullException(nameof(logger)); | ||
| _metricsTracker = metricsTracker ?? throw new ArgumentNullException(nameof(metricsTracker)); | ||
| //_metricsLogger = metricsLogger ?? throw new ArgumentNullException(nameof(metricsLogger)); |
There was a problem hiding this comment.
Removed the metricsLogger dependency here (as metricsLogger depends on metricsPublisher and is causing cyclic dependency). The original code: https://github.com/Azure/azure-functions-host/pull/9741/files#diff-b40529fc8edfa035050463add05ab73052020dd74d615cdd5e939b860e739d5b.
Seems like it was being used to log the OnMetricsDiagnosticEvent. So we need to figure out a way to add it back and refactor it out to remove the cyclic dependency. @mathewc any suggestions here.
There was a problem hiding this comment.
Ah, I see. One way to break this cycle is to employ the Service Locator antipattern as we've done in other places in the code to resolve these, e.g. here.
So you could add an IScriptHostManager ctor param then use it to resolve the metrics logger. You'd define a helper property MetricsLogger that would do the service resolution.
@fabiocav in case he has another idea :)
| @@ -58,6 +58,7 @@ | |||
| <PackageReference Include="Microsoft.AspNetCore.Mvc.NewtonsoftJson" Version="8.0.1" /> | |||
| <PackageReference Include="Microsoft.Azure.AppService.Middleware.Functions" Version="1.5.5" /> | |||
| <PackageReference Include="Microsoft.Azure.AppService.Proxy.Client" Version="2.3.20240307.67" /> | |||
| <PackageReference Include="Microsoft.Azure.Functions.Platform.Metrics.LinuxConsumption" Version="1.0.4" /> | |||
There was a problem hiding this comment.
We need to setup official build pipeline to publish this package. Currently this one was manually uploaded as per: #9741 (comment)
src/WebJobs.Script.WebHost/Metrics/LinuxContainerLegionMetricsPublisher.cs
Show resolved
Hide resolved
| var logger = s.GetService<ILogger<LinuxContainerLegionMetricsPublisher>>(); | ||
| var metricsTracker = s.GetService<ILinuxConsumptionMetricsTracker>(); | ||
| var standbyOptions = s.GetService<IOptionsMonitor<StandbyOptions>>(); | ||
| //var metricsLogger = s.GetService<IMetricsLogger>(); |
There was a problem hiding this comment.
TODO: Once we fix the cyclic ref update this code
| @@ -280,7 +286,15 @@ private static void AddLinuxContainerServices(this IServiceCollection services) | |||
| services.AddSingleton<IMetricsPublisher>(s => | |||
| { | |||
| var environment = s.GetService<IEnvironment>(); | |||
| if (environment.IsFlexConsumptionSku()) | |||
| if (environment.IsV1LinuxConsumptionOnLegion()) | |||
There was a problem hiding this comment.
I think we should rename this env helper to IsLinuxConsumptionOnLegion, which matches IsLinuxConsumptionOnAtlas
| /// </summary> | ||
| /// <param name="environment">The environment to check.</param> | ||
| /// <returns><see cref="true"/> if the sku is Dynamic; otherwise, <see cref="false"/>.</returns> | ||
| public static bool WebsiteSkuIsDynamic(this IEnvironment environment) |
There was a problem hiding this comment.
TBD if we need this - when I did the first PR this was a work in progress and we were thinking we could check the sku in this way to determine
| [InlineData(true, null, ScriptConstants.DynamicSku, true)] | ||
| [InlineData(true, ScriptConstants.DynamicSku, null, true)] | ||
| [Trait(TestTraits.Group, TestTraits.LinuxConsumptionMetricsTests)] | ||
| public void IsV1LinuxConsumptionOnLegion_ReturnsExpectedResult(bool isLinuxConsumptionOnLegion, string websiteSku, string websiteSkuName, bool expectedValue) |
There was a problem hiding this comment.
After rename, fix this to IsLinuxConsumptionOnLegion_ReturnsExpectedResult
Issue describing the changes in this PR
This PR pulls in the new metrics component for Linux Consumption in Legion environments
This is exactly same as: #9741, except for few changes which I highlighted as Qs in comments below.
resolves #issue_for_this_pr
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