Board Review: Azure.Core features for LLC/codegen project

[lmazuel]

Context

The LLC project required some improvement in Azure.Core that are today in Experimental package or preview, that we need to review to GA the associated Azure.Core. This is a necessary step in the story of enabling LLC-based SDK to go GA (we can't GA a LLC based service team that uses previews of Azure.Core).

.NET

• APIView Links:

  • Azure.Core: https://apiview.dev/Assemblies/Review/b7f8a2ca9fad4d93af643366bfee9d2f?diffRevisionId=17448da609b6499bbe0c6916777efa02&doc=False&diffOnly=True
  • Azure.Core.Experimental : https://apiview.dev/Assemblies/Review/e3edef81e94e4ba5b758964703ca611b

• Link to Champion Scenarios/Quickstart samples: https://gist.github.com/annelo-msft/8de841b3d85adec4b2420597542ee936

Java

• APIView Link: https://apiview.dev/Assemblies/Review/8323fedf151f483e80bbb031a0b1dbab/5bc2a841962b45e28aa05485b9ff68e5?diffRevisionId=62ee2a8232534f73b098fe0c8d7e116c&doc=False&diffOnly=True

• Link to Champion Scenarios/Quickstart samples: https://gist.github.com/srnagar/03d4e4f489cbfb01c28bf77cbe149bce

Python

• APIView Link: https://apiview.dev/Assemblies/Review/998d0146014f4305bd4e6054b2eb56aa?diffRevisionId=972708018b464d01a10c8ee0b5267091&doc=False&diffOnly=False
• Link to Champion Scenarios/Quickstart samples: https://gist.github.com/iscai-msft/68b41c28c4ef5f5a7b9946996a99bf04

TypeScript

• APIView Link: https://apiview.dev/Assemblies/Review/977173b08cc541e094f7f55b3c73ec0d
• Link to Champion Scenarios/Quickstart samples: https://gist.github.com/joheredi/28c78af740199704869b7e3f65d96f13

[lilyjma]

scheduled for 10/25 2-4pm pst

[tg-msft]

An issue with RequestOptions.CancellationToken

Evolution (LLC => HLC)

I think there's a plan to evolve LLC methods into HLC methods by taking a signature like:

public class FooClient
{
    public Response DoFoo(string query, string header, RequestContent body, RequestOptions options = default);
}

and adding a strongly-typed overload right next to it like:

public class FooClient
{
    public Response DoFoo(string query, string header, RequestContent body, RequestOptions options = default);

    public Response<FooModel> DoFoo(FooOptions options, CancellationToken cancellationToken = default) =>
        DoFoo(
            options?.Query,
            options?.Header,
            RequestContent.Create(options?.Body),
            new RequestOptions { CancellationToken = cancellationToken});
}

[JsonConverter(...)]
public class FooModel { ... } 

public class FooOptions
{
    public FooModel Body { get; set; }   
    public string Query { get; set; }
    public string Header { get; set; }
}

The benefit of this approach is body-less methods can make RequestOptions required so the compiler is able to bind correctly between the different LLC/HLC overloads.

Devolution (HLC + RequestOptions)

RequestOptions adds a number of important features like customizing error behavior and per-invocation pipeline policies. We want that for our Wave 1 HLCs like Storage and KeyVault too. There's currently no plan to update all the handcrafted artisinal client libraries to use RequestOptions, but we'll support it on a case by case basis using customer feedback.

BlobClient.DeleteIfExists is a good example where we'd like to start using this today. We have a problem where 404s from DeleteIfExists pollute the tracing data collected in Azure Monitor. The WebJobs extensions for Azure Functions cause more of the same. We'd like to make use of RequestOptions to provide a per call classifier on select methods to work around this.

Option 1: Just add RequestOptions

The naïve approach would be simply adding an overload taking RequestOptions for all the power of an LLC method:

public class BlobClient
{
    // Current method:
    public virtual Response<bool> DeleteIfExists(
        DeleteSnapshotsOption snapshotsOption = None,
        BlobRequestConditions conditions = default,
        CancellationToken cancellationToken = default);

    // New overload:
    public virtual Response<bool> DeleteIfExists(
        DeleteSnapshotsOption snapshotsOption,
        BlobRequestConditions conditions,
        RequestOptions options,
        CancellationToken cancellationToken);
}

First I have to make all the parameters required so the compiler will correctly bind the empty parameter list. But then what does it mean when I pass both requestOptions.CancellationToken and the cancellationToken parameter? I think the only clean but potentially not very cheap solution is to create another CancellationTokenSource to link the two of them together. This just feels messy and like we designed RequestOptions without caring up about the HLC devolution story.

Option 2: Add an LLC method

We could add the LLC method as if we'd started there two years ago:

public class BlobClient
{
    // Current method:
    public virtual Response<bool> DeleteIfExists(
        DeleteSnapshotsOption snapshotsOption = None,
        BlobRequestConditions conditions = default,
        CancellationToken cancellationToken = default);

    // New Delete overload instead because that's all we have at the protocol layer
    public virtual Response<bool> Delete(
        RequestOptions options,
        string deleteSnapshots = default,
        string leaseId = default,
        DateTimeOffset? ifModifiedSince = default,
        DateTimeOffset? ifUnmodifiedSince = default,
        ETag ifMatch = default,
        ETag ifNoneMatch = default,
        string ifTags = default,
        BlobDeleteType? blobDeleteType = default);
}

There are too many parameters to make them all required so I moved RequestOptions first in this case. I think this looks pretty awful. It'll be a lot of work for customers to unroll our handwritten models that may have drifted from the protocol as we added convenience.

Option 3: Don't take CancellationToken

We could also add an overload taking RequestOptions but not CancellationToken:

public class BlobClient
{
    // Current method:
    public virtual Response<bool> DeleteIfExists(
        DeleteSnapshotsOption snapshotsOption = None,
        BlobRequestConditions conditions = default,
        CancellationToken cancellationToken = default);

    // New overload:
    public virtual Response<bool> DeleteIfExists(
        DeleteSnapshotsOption snapshotsOption,
        BlobRequestConditions conditions,
        RequestOptions options);
}

The first problem is making it non-optional so the compiler can bind DeleteIfExists() also makes all our other parameters required. It's not a deal breaker, but it's annoying.

Second, I also don't think it's ideal for methods with Options bags. Why wouldn't these RequestOptions also live there instead of a top-level parameter? Here's what this would look like with Download:

public class BlobClient
{
    // Current method:
    public virtual Response<BlobDownloadResult> DownloadContent(
        BlobDownloadOptions options,
        CancellationToken cancellationToken = default);

    // New overload:
    public virtual Response<BlobDownloadResult> DownloadContent(
        BlobDownloadOptions options,
        RequestOptions otherMoreDifferentOptions);
}

public class BlobDownloadOptions
{
    public BlobDownloadOptions();
    public BlobRequestConditions Conditions { get; set; }
    public HttpRange Range { get; set; }
    public DownloadTransactionalHashingOptions TransactionalHashingOptions { get; set; }
}

Option 4: Remove CancellationToken from RequestOptions

This is my preferred approach. It just adds another parameter to Delete:

public class BlobClient
{
    // Hide the current method:
    [EditorBrowsable(EditorBrowsableState.Never)]
    public virtual Response<bool> DeleteIfExists(
        DeleteSnapshotsOption snapshotsOption,
        BlobRequestConditions conditions,
        CancellationToken cancellationToken);

    // New overload that everyone starts seeing:
    public virtual Response<bool> DeleteIfExists(
        DeleteSnapshotsOption snapshotsOption = default,
        BlobRequestConditions conditions = default,
        RequestOptions requestOptions = default,
        CancellationToken cancellationToken = default);
}

And is easily hidden inside Options bags:

public class BlobClient
{
    // No change!
    public virtual Response<BlobDownloadResult> DownloadContent(
        BlobDownloadOptions options,
        CancellationToken cancellationToken = default);
}

public class BlobDownloadOptions
{
    public BlobDownloadOptions();
    public BlobRequestConditions Conditions { get; set; }
    public HttpRange Range { get; set; }
    public DownloadTransactionalHashingOptions TransactionalHashingOptions { get; set; }
    public RequestOptions Request { get; set; }
}

We'd need to think through how splitting off CancellationToken would affect the LLC evolution story, but I don't think it should be very different. You'd still make RequestOptions required which would resolve bindings and then tack an extra CancellationToken on the end.

[tg-msft]

Recording[MS INTERNAL ONLY]

[KrzysztofCwalina]

The issues you describe exist in methods that don't take a model representing the request content. If the do take it, the protocol overload would take RequestContent and so there would not be an ambiguity of overload resolution.

For these relatively uncommon cases, I would prefer to use the workarounds you outlines, i.e. make existing formal parameters required in protocol overloads and live with the fact that there will be operation specific option bags and request-general option bag (RequestOption).

The reason for the preference is that we will have lots of protocol methods all over the SKD and I would like them to be as simple as possible, and I think it will be relatively uncommon for a method to not take models and have optional formal parameters.

[tg-msft]

Recording[MS INTERNAL ONLY]

[lilyjma]

scheduled for 11/15 2-3pm pst

[tg-msft]

Recording[MS INTERNAL ONLY]