How do we set Pipelines options to a Client?

[cataggar]

The draft PR presented one option & other options were discussed in comments.

• pass in Pipeline to ClientBuilder #531

[heaths]

Passing in a transport is about the lowest-level operation our other language SDKs provide. This is useful for testing, or for really customizing the headers, etc. But other pipeline options like number of retries, the retry policy, headers and query parameters logged, how much information is logged, etc., is a fairly established pattern in the other SDKs (though different idiomatically) we should mimic in concept. This is what I did with the azure_core::ClientOptions class as shown in an example here:

azure-sdk-for-rust/sdk/core/src/options.rs

Lines 12 to 17 in c4257f7

	/// ```
	/// use azure_core::{ClientOptions, RetryOptions, TelemetryOptions};
	/// let options: ClientOptions = ClientOptions::default()
	/// .retry(RetryOptions::default().max_retries(10u32))
	/// .telemetry(TelemetryOptions::default().application_id("my-application"));
	/// ```

Was there some specific problems with this? This pattern was also discussed within the v-team back when I submitted the PR. /cc @yoshuawuyts

[yoshuawuyts]

I'm keen to get feedback on the proposal I put out in #531 (comment). What do people think of this direction?

An alternative interface for constructing clients

I see the "pipeline" and "options" parameters as mirrors to the interface issues we faced with "context" and "options" in our endpoint constructors. And I believe we should be able to address these in a similar manner. I'd like to propose we consider constructing clients using an API along these lines instead:

let account = std::env::var("COSMOS_ACCOUNT").unwrap();
let master_key = std::env::var("COSMOS_MASTER_KEY").unwrap();
let auth = AuthorizationToken::primary_from_base64(&master_key)?;

// Create a new Cosmos client with a custom policy
let client = CosmosClient::new(account, auth)
    .retry_policy(ExamplePolicy::new());

This would move all CosmosClientOptions optional parameters into a builder, and add two new methods: retry_policy and call_policy which take an impl Policy each. I feel like this would be a more ergonomic experience, with the benefit of having the flexibility to be able to extend the policies.

[heaths]

If we wanted to eliminate the ClientOptions and conceptually derivative <ClientClass>ClientOptions classes and set options directly on the client, I think that's fine, but we should provide the same setters that exist (or would exist) on ClientOptions now. That would seem idiomatic, and conceptually similar to what Python is doing.

The ClientOptions code as it exists now was more or less modeled after the Azure SDKs for Java since we decided to go with a builder pattern.

[heaths]

@JeffreyRichter any thoughts here based on other native SDKS?

[heaths]

In Python, for example, you provide key/value pairs to set general pipeline options via **kwargs as seen here: https://azuresdkdocs.blob.core.windows.net/$web/python/azure-keyvault-secrets/latest/azure.keyvault.secrets.html#azure.keyvault.secrets.SecretClient. JavaScript uses an options bag, so while technically a separate object with properties (map with key/value pairs), it's conceptually similar.

If we go this route, how do we enforce it? Does ClientOptions become a trait from which all clients should derive so its associated functions just "magically" show up and can be useful?

[JeffreyRichter]

What we want is for Core to define a common set of policies and their options. And then, we also need to let service clients add service-specific policies & options. In languages that support inheritance, Core has an options struct/class with what's in Core and service clients create their own options that derive off the Core options. In go, we don't have inheritance and so we embed Core's options inside the service's client's options as a field.

In Rust, since we can't easily initiate the fields and add new fields in a backward compatible way, we've been using the builder pattern. This means that every service client options needs to wrap Core's Options and re-implement the methods that set individual policy options - this is unfortunate for the Microsoft engineer but is a good customer experience.

[heaths]

@yoshuawuyts also worth taking a look at the original discussion item #300. Perhaps superseded by this, but would be good to consolidate on a single discussion issue (whichever is fine).

[heaths]

One concern I have with hanging client options-setting methods off a client itself (like your retry_policy() method) is something that has come up in archboard reviews: too many APIs on a client make finding the "important methods" (service calls) harder to discover. It also means options can be set after calls are made (or at least give the appearance client options can be changed) and the clients should be immutable - largely stateless, in fact. See our guidelines on this topic.

When we start getting "too many methods" (it's rather subjective) we start creating "subclients" or "operation groups". Our older track 1 libraries had these a lot, and we're still deciding at an archboard level how these will look on our track 2 libraries (all the Azureprefixed libraries, basically, instead of Microsoft.Azure).