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.

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

Linux Consumption metrics publisher for Legion #9741

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

Conversation

mathewc
Copy link
Member

@mathewc mathewc commented Dec 13, 2023

This PR pulls in the new metrics component for Linux Consumption in Legion environments.

Pull request checklist

  • 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)

@mathewc mathewc requested a review from a team as a code owner December 13, 2023 20:19
@mathewc mathewc marked this pull request as draft December 13, 2023 20:19
@@ -24,6 +20,7 @@ public class FlexConsumptionMetricsPublisher : IMetricsPublisher, IDisposable
private readonly ILogger<FlexConsumptionMetricsPublisher> _logger;
private readonly object _lock = new object();
private readonly IFileSystem _fileSystem;
private readonly LegionMetricsFileManager _metricsFileManager;
Copy link
Member Author

Choose a reason for hiding this comment

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

Both Flex and CV1 on Legion use the same file writer for metrics, so I refactored this logic out

{
private readonly ILinuxConsumptionMetricsTracker _metricsTracker;
private readonly LegionMetricsFileManager _metricsFileManager;
private readonly TimeSpan _memorySnapshotInterval = TimeSpan.FromMilliseconds(1000);
Copy link
Member Author

Choose a reason for hiding this comment

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

The intervals for memory and publishing are the same as in LinuxContainerMetricsPublisher, The main change is how this activity is published. Here we publish to the ILinuxConsumptionMetricsTracker component directly, instead of buffering activities and flushing out via http as LinuxContainerMetricsPublisher does.

{
try
{
if (_metricsTracker.TryGetMetrics(out LinuxConsumptionMetrics trackedMetrics))
Copy link
Member Author

@mathewc mathewc Dec 13, 2023

Choose a reason for hiding this comment

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

The tracker returns aggregated metrics based on the activities that were sent to it, based on its own internal aggregation intervals


namespace Microsoft.Azure.WebJobs.Script.WebHost.Models
{
public class ActivityBase
Copy link
Member Author

@mathewc mathewc Dec 13, 2023

Choose a reason for hiding this comment

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

These contracts are shared between CV1 and Flex. I moved these existing types out into the new Microsoft.Azure.Functions.Platform.Metrics.LinuxConsumption library

@@ -241,6 +242,11 @@ private static void AddStandbyServices(this IServiceCollection services)

private static void AddLinuxContainerServices(this IServiceCollection services)
{
if (SystemEnvironment.Instance.IsLinuxConsumptionOnLegion())
{
services.AddLinuxConsumptionMetricsServices();
Copy link
Member Author

Choose a reason for hiding this comment

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

This adds the ILinuxConsumptionMetricsTracker implementation

@@ -75,6 +75,7 @@
<PackageReference Include="Microsoft.Azure.AppService.Middleware.Functions" Version="1.5.4" />
<PackageReference Include="Microsoft.Azure.AppService.Proxy.Client" Version="2.2.20220831.41" />
<PackageReference Include="Microsoft.Azure.Cosmos.Table" Version="1.0.8" />
<PackageReference Include="Microsoft.Azure.Functions.Platform.Metrics.LinuxConsumption" Version="1.0.3" />
Copy link
Member Author

Choose a reason for hiding this comment

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

For this draft PR, I manually uploaded this version of the package to the Azure Functions artifacts feed. An official build pipeline isn't set up for this yet.

@mathewc mathewc marked this pull request as ready for review January 19, 2024 01:56
@mathewc mathewc changed the title [Draft] Linux Consumption metrics publisher for Legion Linux Consumption metrics publisher for Legion Jan 19, 2024
Copy link
Contributor

@anandagopal6 anandagopal6 left a comment

Choose a reason for hiding this comment

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

Will look at PR more in depth soon, leaving initial comment for now since it is a potential issue

/// <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) && string.IsNullOrEmpty(environment.GetEnvironmentVariable(FunctionsMetricsPublishPath));
Copy link
Contributor

Choose a reason for hiding this comment

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

@mathewc is there supposed to be a "!" in front of the null/empty check for FunctionsMetricsPublishPath?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I see that we'll need to change this. I came up with this after initially discussing with Bala, but you're right that in both cases the metrics path will be set, so we need a different discriminator

_metricsTracker = metricsTracker ?? throw new ArgumentNullException(nameof(metricsTracker));
_metricsLogger = metricsLogger ?? throw new ArgumentNullException(nameof(metricsLogger));
_containerName = _environment.GetEnvironmentVariable(EnvironmentSettingNames.ContainerName);
_metricPublishInterval = TimeSpan.FromMilliseconds(metricsPublishIntervalMS ?? 30 * 1000);
Copy link
Contributor

Choose a reason for hiding this comment

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

curious, why is this 30 seconds instead of 5 seconds?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants