Skip to content

Conversation

@amnguye
Copy link
Member

@amnguye amnguye commented Oct 8, 2025

From #52977 @alrod :

"This PR introduces a streamlined approach to zero-to-one scaling logic within the Azure SDK. The current implementation reads and parses all classic monitoring logs in the $log container to extract modified blob paths. However, for scale-out from zero to one, this level of detail is unnecessary.
The updated logic simplifies the process by checking only for the presence of write operations within the last two hours—without downloading or parsing the full log content. This optimization reduces memory consumption and improves performance, especially in environments with high write volumes."

@Copilot Copilot AI review requested due to automatic review settings October 8, 2025 20:34
@github-actions github-actions bot added the Storage Storage Service (Queues, Blobs, Files) label Oct 8, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a lightweight detection mechanism for new write operations in Azure Storage blob monitoring, optimizing the zero-to-one scaling logic. Instead of reading and parsing complete log files, it now checks only for the presence of write operations in metadata within a specified time window.

Key changes:

  • Added a new HasBlobWritesAsync method that checks for write operations without downloading log content
  • Simplified the zero-to-one scaling monitor to use the lightweight detection
  • Updated logging messages and removed debugging-related code

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
BlobLogListener.cs Added HasBlobWritesAsync method for lightweight write detection and made constructor public
BlobScalerMonitorProvider.cs Replaced detailed write enumeration with simple boolean check and updated logging
BlobLogListenerTests.cs Added comprehensive tests for the new HasBlobWritesAsync method with various scenarios

Comment on lines +121 to +123
private static readonly System.Reflection.PropertyInfo NameProp = BlobItemType.GetProperty(nameof(BlobItem.Name))!;
private static readonly System.Reflection.PropertyInfo MetadataProp = BlobItemType.GetProperty(nameof(BlobItem.Metadata))!;

Copy link

Copilot AI Oct 8, 2025

Choose a reason for hiding this comment

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

The null-forgiving operator (!) is used without null checks. If these properties don't exist, this will throw at runtime. Add proper null validation or use more defensive reflection patterns.

Suggested change
private static readonly System.Reflection.PropertyInfo NameProp = BlobItemType.GetProperty(nameof(BlobItem.Name))!;
private static readonly System.Reflection.PropertyInfo MetadataProp = BlobItemType.GetProperty(nameof(BlobItem.Metadata))!;
private static readonly System.Reflection.PropertyInfo NameProp = BlobItemType.GetProperty(nameof(BlobItem.Name));
private static readonly System.Reflection.PropertyInfo MetadataProp = BlobItemType.GetProperty(nameof(BlobItem.Metadata));
static BlobItemFactory()
{
if (NameProp == null)
{
throw new InvalidOperationException($"Property '{nameof(BlobItem.Name)}' not found on type '{BlobItemType.FullName}'.");
}
if (MetadataProp == null)
{
throw new InvalidOperationException($"Property '{nameof(BlobItem.Metadata)}' not found on type '{BlobItemType.FullName}'.");
}
}

Copilot uses AI. Check for mistakes.
DateTime hourCursor = DateTime.UtcNow;
BlobContainerClient containerClient = _blobClient.GetBlobContainerClient(LogContainer);

int processedCount = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this isn't really used and can be removed


await foreach (var page in containerClient
.GetBlobsAsync(traits: BlobTraits.Metadata, prefix: prefix, states: BlobStates.None, cancellationToken: cancellationToken)
.AsPages(pageSizeHint: 200)
Copy link
Member

Choose a reason for hiding this comment

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

How did you choose this 200 - is it a default we use elsewhere?

Copy link
Member

@alrod alrod Oct 8, 2025

Choose a reason for hiding this comment

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

I don’t believe paging is necessary here. When I tested uploading 100K blobs, it only generated three logs. I’m removing the paging logic. The blob log itself is quite large—tens of megabytes.

image

processedCount++;

// Examine metadata only if present.
if (blob.Metadata is not null &&
Copy link
Member

Choose a reason for hiding this comment

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

nit: could make this a little more succinct:

if (blob.Metadata?.TryGetValue(LogType, out string logType) &&
    logType?.Contains("write", StringComparison.OrdinalIgnoreCase))
{
    return true;
}

Copy link
Member

Choose a reason for hiding this comment

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

This does not compile. Copilot:

The shown replacement is problematic for multiple reasons:

  1. Null-conditional invocation result type: blob.Metadata?.TryGetValue(...) returns bool? (nullable bool). You can’t use && with a bool? left operand. Won’t compile.
  2. API availability: string.Contains(string, StringComparison) is NOT available in .NET Standard 2.0. This project multi-targets .NET 8 and .NET Standard 2.0, so that overload breaks the netstandard2.0 build. The original IndexOf(..., StringComparison.OrdinalIgnoreCase) works on both.
  3. Semantics / safety: The original guarded against null or empty logType explicitly. Your version relies on logType?.Contains, which is fine, but coupled with point (1) & (2) it fails anyway.
  4. Consistency: Elsewhere in this file (GetLogsWithPrefixAsync) you have logType.Contains("write") without a StringComparison, which is culture-sensitive. The original pattern (IndexOf + StringComparison.OrdinalIgnoreCase) is the safer, consistent approach.
    Recommended fixed version (compiles for both targets and remains clear):
if (blob.Metadata is not null &&
    blob.Metadata.TryGetValue(LogType, out var logType) &&
    !string.IsNullOrEmpty(logType) &&
    logType.IndexOf("write", StringComparison.OrdinalIgnoreCase) >= 0)
{
    return true;
}


public static BlobItem Create(string name, IDictionary<string, string> metadata)
{
var ctor = BlobItemType.GetConstructor(BindingFlags.Instance | BindingFlags.NonPublic, binder: null, types: Type.EmptyTypes, modifiers: null)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if @amnguye knows a better way to mock this stuff?

Copy link
Member

Choose a reason for hiding this comment

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

BlobItem does not have public ctor. It seems there is no other way.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you use the BlobsModelFactory we have some constructors of the BlobItem you can mock.

For example:

BlobItem blobItem = BlobsModelFactory.BlobItem(
    name: result.Item1,
    properties: BlobsModelFactory.BlobItemProperties(
        accessTierInferred: true,
        contentLength: 1024,
        blobType: BlobType.Block,
        eTag: new ETag("etag")));
blobHierarchyItems.Add(new BlobHierarchyItem(default, blobItem));

Copy link
Member

Choose a reason for hiding this comment

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

Thanks - yes that's what I recalled in the past for other SDK types - there was a model factory pattern that could be used

@alrod alrod requested a review from mathewc October 8, 2025 23:26
@alrod alrod closed this Oct 21, 2025
@alrod
Copy link
Member

alrod commented Oct 21, 2025

Closed in favor: #53263

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Storage Storage Service (Queues, Blobs, Files)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants