Attach client-level span fields

[heaths]

This isn't quite what we want because the fields are still buried in a "self" field, despite numerous attempts to flatten them. Opened tokio-rs/tracing#3124 to track a feature request (or similar).

We have options but most are rather error prone. Perhaps the best way is our own attribute macro that would expand to #[tracing::instrument] with appropriate defaults to attach client-level fields like az.namespace.

We could also write a helper function or two to just wrap the whole thing, which would also give us better control over the error or return value as well, like attaching error.type per our guidelines.

[heaths]

@RickWinter @JeffreyRichter @LarryOsterman @ronniegeraghty @jhendrixMSFT @analogrelay I've been playing around with the tracing crate but it's difficult to easily (for devs) to attach client-level fields to all client method calls. I've opened a feature request but it may be a long shot, at least in the short term. It may be better to create a helper function that would take a closure (or, like tracing's instrument() "extension method", work on a future) and attach data as needed. You can see an example of what it would almost look like in clients.rs where I attempt (and in this draft it no longer works; see main for a working version) to check if we already traced this client. This supports our guideline https://azure.github.io/azure-sdk/general_implementation.html#general-tracing-suppress-client-spans-for-inner-methods.

It's not as elegant as the #[instrument] attribute macro - though perhaps we can write our own that expands to that with our own fields - but not much different than some of our other languages' (most if not all) SDKs that have code in every method - generated or hand-written - to define and start a span. It could look something like:

impl SecretClient {
    const NAMESPACE: &str = "Microsoft.KeyVault";

    #[client_method(fields(az.keyvault.secret.name = name, az.keyvault.secret.version = version))]
    pub async fn get_secret(&self, name: &str, version: Option<&str>, options: Option<GetSecretMethodOptions>) -> Result<Response<KeyVaultSecret>> {
        todo!()
    }
}

Which would effectively expand to:

impl SecretClient {
    const NAMESPACE: &str = "Microsoft.KeyVault";

    #[::tracing::instrument(
        target = "SecretClient::get_secret",
        skip_all,
        fields(
            az.namespace = Self::NAMESPACE,
            az.client = stringify!(SecretClient),
            az.keyvault.secret.name = name,
            az.keyvault.secret.version = version,
        ),
        err,
    )]
    pub async fn get_secret(&self, name: &str, version: Option<&str>, options: Option<GetSecretMethodOptions>) -> Result<Response<KeyVaultSecret>> {
        todo!()
    }
}

[analogrelay]

I think this makes a lot of sense. I can also think of other functionality we might want to embed through macros here, like grabbing fields off the client type itself (which, if already supported by tracing::instrument can just be passed through).

[analogrelay]

I wonder if we should have a trait for service clients (including sub-clients) and a derive macro. Then we could have a blanket impl of impl<T> Valuable for T where T: azure_core::Client¹. Clients could look something like this:

#[derive(Client)]
#[azure(namespace = "Microsoft.KeyVault")]
pub struct SecretClient {
}

Footnotes

1. Or, if that doesn't work, derive Valuable in the derive macro expansion. ↩

[heaths]

I was thinking something along those lines last night as well. 😄

All this might change if we instead use the opentelemetry crates. I need to look more into those, but internal conversations are happening and Microsoft at large has been contributing to those much like symcrypt we'll use as needed.