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.

Sign up for GitHub

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

[Draft] Minimize noisy Storage exceptions via DPG methods #27645

Closed
wants to merge 2 commits into from
+1,044 −117
Closed

[Draft] Minimize noisy Storage exceptions via DPG methods #27645

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
020780c
[Draft] Minimize noisy Storage exceptions via DPG methods
tg-msft Mar 17, 2022
5850bfe
Properly classify non-errors
tg-msft Mar 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions sdk/core/Azure.Core/src/Shared/RequestContextExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System.Threading;

namespace Azure.Core
{
/// <summary>
/// Shared source helpers for <see cref="RequestContext"/>.
/// </summary>
internal static class RequestContextExtensions
{
/// <summary>
/// Singleton RequestContext to use when we don't have an instance.
/// </summary>
public static RequestContext Empty { get; } = new RequestContext();

/// <summary>
/// Create a RequestContext instance from a CancellationToken.
/// </summary>
/// <param name="cancellationToken">The cancellation token.</param>
/// <returns>
/// A RequestContext wrapping the cancellation token.
/// </returns>
public static RequestContext ToRequestContext(this CancellationToken cancellationToken) =>
cancellationToken == CancellationToken.None ?
Empty :
new() { CancellationToken = cancellationToken };

/// <summary>
/// Link a CancellationToken with an existing RequestContext.
/// </summary>
/// <param name="context">The RequestContext.</param>
/// <param name="cancellationToken">A cancellation token.</param>
public static void Link(this RequestContext context, CancellationToken cancellationToken)
{
if (cancellationToken == CancellationToken.None)
{
// Don't link if there's nothing there
}
else if (context.CancellationToken == CancellationToken.None)
{
// Don't link if we can just replace
context.CancellationToken = cancellationToken;
}
else
{
// Otherwise link them together
CancellationTokenSource source = CancellationTokenSource.CreateLinkedTokenSource(
context.CancellationToken,
cancellationToken);
context.CancellationToken = source.Token;
}
}
}
}
2 changes: 2 additions & 0 deletions sdk/storage/Azure.Storage.Blobs/api/Azure.Storage.Blobs.netstandard2.0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1578,7 +1578,9 @@ public BlobLeaseClient(Azure.Storage.Blobs.Specialized.BlobBaseClient client, st
protected virtual Azure.Storage.Blobs.BlobContainerClient BlobContainerClient { get { throw null; } }
public virtual string LeaseId { get { throw null; } }
public System.Uri Uri { get { throw null; } }
public virtual Azure.Response<Azure.Storage.Blobs.Models.BlobLease> Acquire(System.TimeSpan duration, Azure.RequestConditions conditions, Azure.RequestContext context) { throw null; }
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we determine that we want to add RequestContext to HLC methods vs using it internally.
I.e. why this instead of AcquireIfNotYetAcuired API that uses RequestContext internally?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it's not really doing AcquireIfNotYetAcquired. It's doing AcquireIfBlobNotFoundOrContainerNotFoundOrLeaseAlreadyAcquired and we don't want to add an overload for that specific, non-general scenario.

public virtual Azure.Response<Azure.Storage.Blobs.Models.BlobLease> Acquire(System.TimeSpan duration, Azure.RequestConditions conditions = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual System.Threading.Tasks.Task<Azure.Response<Azure.Storage.Blobs.Models.BlobLease>> AcquireAsync(System.TimeSpan duration, Azure.RequestConditions conditions, Azure.RequestContext context) { throw null; }
public virtual System.Threading.Tasks.Task<Azure.Response<Azure.Storage.Blobs.Models.BlobLease>> AcquireAsync(System.TimeSpan duration, Azure.RequestConditions conditions = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual Azure.Response<Azure.Storage.Blobs.Models.BlobLease> Break(System.TimeSpan? breakPeriod = default(System.TimeSpan?), Azure.RequestConditions conditions = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual System.Threading.Tasks.Task<Azure.Response<Azure.Storage.Blobs.Models.BlobLease>> BreakAsync(System.TimeSpan? breakPeriod = default(System.TimeSpan?), Azure.RequestConditions conditions = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
Expand Down
1 change: 1 addition & 0 deletions sdk/storage/Azure.Storage.Blobs/src/Azure.Storage.Blobs.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
<Compile Include="$(AzureCoreSharedSources)ForwardsClientCallsAttribute.cs" LinkBase="SharedCore" />
<Compile Include="$(AzureCoreSharedSources)HashCodeBuilder.cs" LinkBase="SharedCore" />
<Compile Include="$(AzureCoreSharedSources)NoBodyResponseOfT.cs" LinkBase="SharedCore" />
<Compile Include="$(AzureCoreSharedSources)RequestContextExtensions.cs" LinkBase="SharedCore" />
</ItemGroup>
<ItemGroup>
<Compile Include="$(AzureStorageSharedSources)ClientsideEncryption\*.cs" LinkBase="Shared" />
Expand Down
66 changes: 45 additions & 21 deletions sdk/storage/Azure.Storage.Blobs/src/BlobBaseClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4442,39 +4442,63 @@ private async Task<Response<bool>> ExistsInternal(
nameof(BlobBaseClient),
message:
$"{nameof(Uri)}: {Uri}");

string operationName = $"{nameof(BlobBaseClient)}.{nameof(Exists)}";

DiagnosticScope scope = ClientConfiguration.ClientDiagnostics.CreateScope($"{nameof(BlobBaseClient)}.{nameof(Exists)}");
annelo-msft marked this conversation as resolved.
Show resolved Hide resolved
try
{
Response<BlobProperties> response = await GetPropertiesInternal(
conditions: default,
async: async,
cancellationToken: cancellationToken,
operationName)
.ConfigureAwait(false);
scope.Start();

return Response.FromValue(true, response.GetRawResponse());
}
catch (RequestFailedException storageRequestFailedException)
when (storageRequestFailedException.ErrorCode == BlobErrorCode.BlobNotFound
|| storageRequestFailedException.ErrorCode == BlobErrorCode.ContainerNotFound)
{
return Response.FromValue(false, default);
}
catch (RequestFailedException storageRequestFailedException)
when (storageRequestFailedException.ErrorCode == BlobErrorCode.BlobUsesCustomerSpecifiedEncryption)
{
return Response.FromValue(true, default);
// Get the blob properties, but don't throw any exceptions
RequestContext context = new()
{
ErrorOptions = ErrorOptions.NoThrow,
Copy link
Member

Choose a reason for hiding this comment

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

This will suppress an exception from being thrown from within GetProperties, but error logging and tracing will still occur in the pipeline. In order to suppress those as well, you'll need to add classifiers using context.AddClassifier(...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sigh... That's what I get for rushing and only checking the Storage tests. I updated and verified that distributed tracing is clean too.

Current:
Current

Fixed:
Fixed

#define Fixed

using Azure;
using Azure.Core;
using Azure.Monitor.OpenTelemetry.Exporter;
using Azure.Storage.Blobs;
using Azure.Storage.Blobs.Models;
using Azure.Storage.Blobs.Specialized;
using OpenTelemetry.Trace;
using System.Diagnostics;

// Settings
const string StorageConnStr = "...";
const string AppInsightsConnStr = "...";
const string Name =
#if !Fixed
    "Current";
#else
    "Fixed";
#endif

// Collect telemetry in Azure Monitor
AppContext.SetSwitch("Azure.Experimental.EnableActivitySource", true);
using TracerProvider telemetry =
    OpenTelemetry.Sdk
    .CreateTracerProviderBuilder()
    .AddSource(Name, "Azure.*")
    .AddAzureMonitorTraceExporter(options => options.ConnectionString = AppInsightsConnStr)
    .Build();
using ActivitySource source = new(Name);
using Activity? group = source.StartActivity(Name)!;


// Check if a fake blob exists
BlobClient fake = new BlobClient(StorageConnStr, "fakecontainer", "fakeblob");
Console.WriteLine($"Exists: {fake.Exists().Value}");


// Try acquiring a lease from a fake blob
#if !Fixed

try
{
    BlobLease lease = fake.GetBlobLeaseClient().Acquire(TimeSpan.FromSeconds(15));
    Console.WriteLine($"Lease: {lease.LeaseId}");
}
catch (RequestFailedException ex)
{
    Console.WriteLine($"Failed to obtain lease!\n{ex}");
}

#else

RequestContext context =
    new RequestContext() { ErrorOptions = ErrorOptions.NoThrow }
    .AddClassifier(404, BlobErrorCode.BlobNotFound.ToString(), false)
    .AddClassifier(404, BlobErrorCode.ContainerNotFound.ToString(), false)
    .AddClassifier(409, BlobErrorCode.LeaseAlreadyPresent.ToString(), false);
Response<BlobLease> response =
    fake.GetBlobLeaseClient().Acquire(
        TimeSpan.FromSeconds(15),
        conditions: null,
        context);
Response raw = response.GetRawResponse();
if (raw.Status == 201)
{
    Console.WriteLine($"Lease: {response.Value.LeaseId}");
}
else
{
    Console.WriteLine($"Failed to obtain lease!\n{raw.Content.ToString()}");
}

#endif

CancellationToken = cancellationToken
};
Response response = async ?
await BlobRestClient.GetPropertiesAsync(context: context).ConfigureAwait(false) :
BlobRestClient.GetProperties(context: context);

// If the response wasn't an error, then the blob exists
if (!response.IsError)
{
return Response.FromValue(true, response);
}

// Otherwise it depends on the error
string code = response.GetErrorCode();
if (code == BlobErrorCode.BlobNotFound ||
code == BlobErrorCode.ContainerNotFound)
{
// If the failure was just "we can't find the blob" then
// we'll say it doesn't exist instead of throwing an exception
return Response.FromValue(false, response);
}
else if (code == BlobErrorCode.BlobUsesCustomerSpecifiedEncryption)
{
return Response.FromValue(true, response);
}
else
{
// If we get any other failure (auth, throttling, etc.) then
// we'll still throw an exception
RequestFailedException ex = async ?
await ClientConfiguration.ClientDiagnostics.CreateRequestFailedExceptionAsync(response).ConfigureAwait(false) :
ClientConfiguration.ClientDiagnostics.CreateRequestFailedException(response);
// Can't simply do the following because of https://github.com/Azure/azure-sdk-for-net/issues/27614
// ex = new RequestFailedException(response);

throw ex;
}
}
catch (Exception ex)
{
ClientConfiguration.Pipeline.LogException(ex);
scope.Failed(ex);
throw;
}
finally
{
ClientConfiguration.Pipeline.LogMethodExit(nameof(BlobBaseClient));
scope.Dispose();
}
}
}
Expand Down
56 changes: 38 additions & 18 deletions sdk/storage/Azure.Storage.Blobs/src/BlobContainerClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1595,28 +1595,48 @@ private async Task<Response<bool>> ExistsInternal(
{
using (ClientConfiguration.Pipeline.BeginLoggingScope(nameof(BlobContainerClient)))
{
ClientConfiguration.Pipeline.LogMethodEnter(
nameof(BlobContainerClient),
message:
$"{nameof(Uri)}: {Uri}");

ClientConfiguration.Pipeline.LogMethodEnter(nameof(BlobContainerClient), message: $"{nameof(Uri)}: {Uri}");
DiagnosticScope scope = ClientConfiguration.ClientDiagnostics.CreateScope($"{nameof(BlobContainerClient)}.{nameof(Exists)}");
scope.Start();

try
{
Response<BlobContainerProperties> response = await GetPropertiesInternal(
conditions: null,
async: async,
cancellationToken: cancellationToken)
.ConfigureAwait(false);
scope.Start();

return Response.FromValue(true, response.GetRawResponse());
}
catch (RequestFailedException storageRequestFailedException)
when (storageRequestFailedException.ErrorCode == BlobErrorCode.ContainerNotFound)
{
return Response.FromValue(false, default);
// Get the container properties without throwing any exceptions
RequestContext context = new()
{
ErrorOptions = ErrorOptions.NoThrow,
CancellationToken = cancellationToken
};
Response response = async ?
await ContainerRestClient.GetPropertiesAsync(context: context).ConfigureAwait(false) :
ContainerRestClient.GetProperties(context: context);

// If the response wasn't an error, then the container exists
if (!response.IsError)
{
return Response.FromValue(true, response);
}

// Otherwise it depends on the error
string code = response.GetErrorCode();
if (code == BlobErrorCode.ContainerNotFound)
{
// If the failure was just "we can't find the container" then
// we'll say it doesn't exist instead of throwing an exception
return Response.FromValue(false, response);
}
else
{
// If we get any other failure (auth, throttling, etc.) then
// we'll still throw an exception
RequestFailedException ex = async ?
await ClientConfiguration.ClientDiagnostics.CreateRequestFailedExceptionAsync(response).ConfigureAwait(false) :
ClientConfiguration.ClientDiagnostics.CreateRequestFailedException(response);
// Can't simply do the following because of https://github.com/Azure/azure-sdk-for-net/issues/27614
// ex = new RequestFailedException(response);

throw ex;
}
}
catch (Exception ex)
{
Expand Down
Loading