-
Notifications
You must be signed in to change notification settings - Fork 454
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Linux Consumption metrics publisher for Legion #10750
Open
vivekjilla
wants to merge
6
commits into
dev
Choose a base branch
from
user/vijilla/legion-cv1-metrics
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
ef86f05
Linux Consumption metrics publisher for Legion
vivekjilla d8c9b85
nit
vivekjilla 20d43c1
Removing cyclic dependency between metricslogger and metricspublisher
vivekjilla bdfe28f
Remove Cyclic Dependency between metrics logger and publisher
jainharsh98 16aa650
Fixing Unit test case failures
jainharsh98 42a846e
Disabling events not used warnings
jainharsh98 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
109 changes: 109 additions & 0 deletions
109
src/WebJobs.Script.WebHost/Metrics/LegionMetricsFileManager.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,109 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the MIT License. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.IO; | ||
using System.IO.Abstractions; | ||
using System.Linq; | ||
using System.Threading.Tasks; | ||
using Microsoft.Azure.WebJobs.Script.Diagnostics.Extensions; | ||
using Microsoft.Extensions.Logging; | ||
using Newtonsoft.Json; | ||
|
||
namespace Microsoft.Azure.WebJobs.Script.WebHost.Metrics | ||
{ | ||
public class LegionMetricsFileManager | ||
{ | ||
private readonly IFileSystem _fileSystem; | ||
private readonly int _maxFileCount; | ||
private readonly ILogger _logger; | ||
|
||
public LegionMetricsFileManager(string metricsFilePath, IFileSystem fileSystem, ILogger logger, int maxFileCount) | ||
{ | ||
_fileSystem = fileSystem ?? throw new ArgumentNullException(nameof(fileSystem)); | ||
_logger = logger ?? throw new ArgumentNullException(nameof(logger)); | ||
MetricsFilePath = metricsFilePath; | ||
_maxFileCount = maxFileCount; | ||
} | ||
|
||
internal string MetricsFilePath { get; set; } | ||
|
||
private bool PrepareDirectoryForFile() | ||
{ | ||
if (string.IsNullOrEmpty(MetricsFilePath)) | ||
{ | ||
return false; | ||
} | ||
|
||
// ensure the directory exists | ||
_fileSystem.Directory.CreateDirectory(MetricsFilePath); | ||
|
||
var metricsDirectoryInfo = _fileSystem.DirectoryInfo.FromDirectoryName(MetricsFilePath); | ||
var files = metricsDirectoryInfo.GetFiles().OrderBy(p => p.CreationTime).ToList(); | ||
|
||
// ensure we're under the max file count | ||
if (files.Count < _maxFileCount) | ||
{ | ||
return true; | ||
} | ||
|
||
// we're at or over limit | ||
// delete enough files that we have space to write a new one | ||
int numToDelete = files.Count - _maxFileCount + 1; | ||
var filesToDelete = files.Take(numToDelete).ToArray(); | ||
|
||
_logger.LogDebug($"Deleting {filesToDelete.Length} metrics file(s)."); | ||
|
||
foreach (var file in filesToDelete) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could benefit from a Parrallel.ForEach - but this is just a code move so no worries. |
||
{ | ||
try | ||
{ | ||
file.Delete(); | ||
} | ||
catch (Exception ex) when (!ex.IsFatal()) | ||
{ | ||
// best effort | ||
_logger.LogError(ex, $"Error deleting metrics file '{file.FullName}'."); | ||
} | ||
} | ||
|
||
files = metricsDirectoryInfo.GetFiles().OrderBy(p => p.CreationTime).ToList(); | ||
|
||
// return true if we have space for a new file | ||
return files.Count < _maxFileCount; | ||
} | ||
|
||
public async Task PublishMetricsAsync(object metrics) | ||
{ | ||
string fileName = string.Empty; | ||
|
||
try | ||
{ | ||
bool metricsPublishEnabled = !string.IsNullOrEmpty(MetricsFilePath); | ||
if (metricsPublishEnabled && !PrepareDirectoryForFile()) | ||
{ | ||
return; | ||
} | ||
|
||
string metricsContent = JsonConvert.SerializeObject(metrics); | ||
_logger.PublishingMetrics(metricsContent); | ||
|
||
if (metricsPublishEnabled) | ||
{ | ||
fileName = $"{Guid.NewGuid().ToString().ToLower()}.json"; | ||
string filePath = Path.Combine(MetricsFilePath, fileName); | ||
|
||
using (var streamWriter = _fileSystem.File.CreateText(filePath)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: can use concise using statement: using var streamWriter = _fileSystem.File.CreateText(filePath);
await streamWriter.WriteAsync(metricsContext); |
||
{ | ||
await streamWriter.WriteAsync(metricsContent); | ||
} | ||
} | ||
} | ||
catch (Exception ex) when (!ex.IsFatal()) | ||
{ | ||
// TODO: consider using a retry strategy here | ||
_logger.LogError(ex, $"Error writing metrics file '{fileName}'."); | ||
} | ||
} | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What package required adding this feed? I would prefer we use an existing feed if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feed was added to test it with the package Microsoft.Azure.Functions.Platform.Metrics.LinuxConsumption for internal consumption. Please refer to #10750 (comment). Is it okay to push it in the tempStaging or PreRelease feed?