tracing sdk operations

[rbtcollins]

I'm happy to put up a patch for this, but thought it should be discussed first.

The problem to be solved is getting visibility into HTTP requests made by the azure SDK.

The obvious thing to do is add a dependency on tracing, and call #[tracing::instrument] on execute_request. Perhaps with skip_all as a parameter, to avoid logging the request.

However this isn't particularly great for being able to categorise things, and it won't use standard otel metadata nor the http status etc, unless more work is done.

We use https://crates.io/crates/reqwest-tracing to do 2 things:

1. add a span with otel standard metadata to each request

2. trigger otel trace propogation to the next service.

3. is probably less relevant to azure (unless azure has a service like AWS X-Ray, where customer traces can integrate deeply) ? However the DisableOtelPropagation extension can be used to disable that.

But 1) is really quite nice - and it means less to maintain.

An alternative approach would be to just implement HTTPClient on a newtype wrapper of ReqwestMiddlewareClient myself - which I haven't tested yet - but that would need a way of injecting that into the various factories like DefaultAzureCredentialBuilder to keep ergnomics nice, and is going to require keeping the implementation in sync with the reqwest one in the sdk

[rbtcollins]

One friction point in an external implementation of HttpClient: try_from_method is not exposed, along with to_headers and try_from_status.

If those were From / TryFrom implementations, that would expose them quite cleanly, I think.

[rbtcollins]

... and PinnedStream is private

[rbtcollins]

As a comparison point, the google rust sdk explicitly uses request-middleware : https://github.com/yoshidan/google-cloud-rust/tree/main/storage#passing-a-custom-reqwest-middleware-cliemt

[heaths]

Thank you for your report. Since we are working on official, supported crates and have no plans to release further versions of the previously unsupported crates, we have decided to close this issue. Some older issues were migrated to the new crates if they seemed relevant. If you feel that this issue was relevant even to newer code that has been or will be rewritten, please reopen and comment as to why you consider it relevant.

[rbtcollins]

Where are these new crates? As you can see from this bug report I opened it and got no feedback or discussion to allow acting on it ...
Also it is very odd to suggest I reopen it when reopening is disabled:

[heaths]

@weshaggard why can't users - especially those who opened the issue - reopen? They've always been able to prior.

@rbtcollins this was flagged as "previous versions" because it was opened against our unsupported, unofficial crates. This was called out in the root README.md. They were never supported, nor will they ever be. We have started almost complete fresh with official crates, such as azure_core 0.22.0 up on crates.io now. The old source was moved to the legacy branch.

But implementing an HttpClient trait is more or less the same: you get a Request object which does give you access to headers, and return a Response which also has headers. If using reqwest, I recommend taking a look at https://github.com/Azure/azure-sdk-for-rust/blob/main/sdk/typespec/typespec_client_core/src/http/clients/reqwest.rs for an example. But if you want to add middleware, you do that by implementing Policy e.g.,

azure-sdk-for-rust/sdk/typespec/typespec_client_core/src/http/policies/custom_headers.rs

Line 28 in b4307af

impl Policy for CustomHeadersPolicy {

. This is how all the other Azure SDK languages do it as well - in concept at least. You add those to the ClientOptions that each HTTP client takes.

But tracing in the official, supported crates is ongoing. We already have issues opened and will get to them soon. We're currently discussing whether we'll even using the tracing crate, the opentelemetry crate instead, or write an abstraction like most other Azure SDK languages have done. It likely won't be ready by our upcoming beta.2 but will certainly be done before we GA.

Since we already have issues tracking telemetry e.g., #1881, I don't see a need to reopen this one, but I did open #2378 to track adding a pipeline policy example or two.

[rbtcollins]

@heaths thank you! If I understand correctly, the new crates are in this same repository?

If I interpret what you say, you're saying the recommended path is the one from my last paragraph ? (

An alternative approach would be to just implement HTTPClient on a newtype wrapper of ReqwestMiddlewareClient myself - which I haven't tested yet - but that would need a way of injecting that into the various factories like DefaultAzureCredentialBuilder to keep ergnomics nice, and is going to require keeping the implementation in sync with the reqwest one in the sdk

)

Thank you for opening the new bug!

[heaths]

Yes, you just need to implement HttpClient and pass it into each client's options e.g.,

let my_http_client = new_my_http_client(); // assumes returns an Arc<dyn HttpClient>

let mut options = SecretClientOptions::default();
options.client_options.transport = Some(TransportOptions::new(my_http_client.clone());
let client = SecretClient::new("https://my-vault.vault.azure.net", credential.clone(), Some(options));