Azure / azure-sdk-for-rust

This repository is for active development of the *unofficial* Azure SDK for Rust. This repository is *not* supported by the Azure SDK team.
MIT License
692 stars 237 forks source link

Add X-RateLimit-* headers used by ADO API #1510

Closed reillysiemens closed 8 months ago

reillysiemens commented 9 months ago

The Azure DevOps Rust API utilizes header types provided by the azure_core crate, but azure_core::headers does not provide HeaderNames for the following headers which are defined in the Azure DevOps documentation.

This PR adds these types as a convenience for upstream consumers of the Azure DevOps Rust API.

demoray commented 9 months ago

Is there a reason these need to live in this crate rather than within azure_devops_rust_api? I don't have a strong position either way.

CCing @johnbatty , as the primary contributor to azure_devops_rust_api.

reillysiemens commented 9 months ago

@demoray, I don't think there's a strong reason that these headers need to belong in this crate rather than the azure_devops_rust_api, but I don't think any other headers are exposed by that crate except by revealing the raw Response type (e.g. as_raw_response). That just directly exposes an azure_core response type and by extension the headers.

As a consumer it feels easier if they're exposed alongside existing headers like Retry-After, but I think there's a good argument to be made that this belongs in the ADO code.

I'm happy either way, TBH, as long as each instance of client code doesn't have to do their own

const X_RATELIMIT_RESOURCE: HeaderName = HeaderName::from_static("x-ratelimit-resource");
kyle-rader-msft commented 9 months ago

From looking at few other Azure rate limiting docs, it doesn't look like we have these headers consistently across Azure :(. So, I'd support moving these the Azure Devops SDK as they do seem to be specific to ADO. For example, MS Graph has a different approach it seems returning 429's once rate limiting is hit https://learn.microsoft.com/en-us/graph/throttling

reillysiemens commented 9 months ago

Really, I'd hope for rate limiting to be handled as part of the Azure DevOps Rust SDK (or to at least expose methods for various strategies), but as it stands I think folks have to do that manually. I think that requires getting raw azure_core response headers. Would love to be wrong about that though.

johnbatty commented 9 months ago

@reillysiemens If, as it seems, these headers are specific to Azure DevOps then I agree they should probably be defined in the azure_devops_rust_api as Brian suggests. Although as you point out it does just make it slightly more tedious for a user who will have to locate and include some extra file with the definitions.

As you mention, rate limiting would ideally be implemented/included in the azure_devops_rust_api. I think this would be best implemented with a Pipeline Policy - I'll take a look into what it would involve.

Do you have a particular scenario/API where you are currently experiencing rate limiting?

cataggar commented 8 months ago

I agree that they should go in azure_devops_rust_api and not azure_core.