ringcentral / RingCentral.Net

RingCentral SDK for .NET
MIT License
19 stars 26 forks source link

API limit information is not exposed #17

Closed BryceBarbara closed 4 years ago

BryceBarbara commented 4 years ago

Most valid responses from the API return with headers which contain useful information about the current state of API limits.

Without this info, it makes it very difficult to anticipate 429 Too Many Request errors if two competing systems are using the same user account through the API.

tylerlong commented 4 years ago

Most of users don't need headers information. So this SDK by default doesn't return it.

For those who need headers, try this: https://github.com/ringcentral/RingCentral.Net/blob/master/RingCentral.Tests/LowLevelApiTest.cs#L59-L70

tylerlong commented 4 years ago

I've added doc: https://github.com/ringcentral/RingCentral.Net#how-to-access-headers

BryceBarbara commented 4 years ago

Thanks for the quick response Tyler!

Although I agree that technically we can access the headers if we handle all response processing ourselves, I feel like the point of the SDK is being missed if that's truly the solution.

Not only does it mean that there will be different implementations for each project, it also removes half of the SDK's value since it would only be used to prepare the requests at that point.

It's almost as if a car didn't have a fuel gauge and instead of adding one, the car manufacturer suggested that users just make their own engine for the car that supports fuel gauges.

BryceBarbara commented 4 years ago

And just to touch on your point about most users not needing the headers, with API limits as low as RingCentral's, I'd have to disagree.

If the info is important enough for every response to contain it, it seems like it should be important enough to officially support.

tylerlong commented 4 years ago

Are you suggesting something like this:

var r = await rc.Restapi().Account("~").Extension("~").Get();
var headers = r.headers
var extInfo = r.body

?

BryceBarbara commented 4 years ago

Exposing the raw headers isn't ideal since we'd still need parse them manually.

I was imagining some sort of property named something like ApiLimits that all the response objects have. That ApiLimits object would look something like this:

struct ApiLimitsInformation {
  public string LimitGroup { get; set; }
  public TimeSpan LimitWindow { get; set; }
  public int TotalLimit { get; set; }
  public int RemainingCalls { get; set; }
}

Let me know if that doesn't make sense. I'm happy to clarify in some other way if needed!

tylerlong commented 4 years ago

There is another way, not sure you like it. https://github.com/ringcentral/RingCentral.Net/blob/master/RingCentral.Tests/AfterCallTest.cs

It's listener pattern so for each http call you can get headers.

BryceBarbara commented 4 years ago

It still requires us to parse the headers ourselves and it's also not idomatic C#.

Here's a boiled down example of why I think it's important to give access to it via the response objects.

var listOfCallIds = this.getSomeArrayFullOfTelephonyIds();
var responses = new List<SomeType>();

foreach (var callId in listOfCallIds) {
  var response = await this._rc.Restapi().Account("~").Extension("~").CallLog(callId).Get();
  responses.Add(response);

  if (response.ApiLimits.RemainingCalls < 1) {
    // Wait some time to avoid getting rate limited
    await Task.Delay(1000 * 61);
  } 
}
tylerlong commented 4 years ago

The overall idea is good. But I don't like the idea of adding ApiLimitsInformation to the response object. Because that information doesn't belong to the object. And the information is temporary only. RemainingCalls only makes sense at that moment, it will become out-of-date very soon.

tylerlong commented 4 years ago

It still requires us to parse the headers ourselves and it's also not idomatic C#.

Yes it does. But you can handle it in a central place (in the event handler) instead of modifying code here and there.

It is idiomatic C#. Check this: C# events.

I am not saying it's the best solution for you. It's just an option FYI.

tylerlong commented 4 years ago

Some code snippets currently in my mind:

// this could be a global object, so that you can share it in your app
var rateLimitTracking = new Dictionary<string, int>{
  { 'Light', 10 }
  { 'Medium', 10 }
  { 'Heavy', 10 }
  { 'Auth', 10 }
}
void EventHandler(object sender, HttpCallEventArgs eventArgs)
{
   var rateLimitGroup = eventArgs.httpResponseMessage.Headers
        .First(i => i.Key == "X-Rate-Limit-Group").Value.First();
   var rateLimitRemaining =int.Parse(eventArgs.httpResponseMessage.Headers
        .First(i => i.Key == "X-Rate-Limit-Remaining").Value.First());
   rateLimitTracking[rateLimitGroup] = rateLimitRemaining
}
rc.AfterHttpCall += EventHandler;

var listOfCallIds = this.getSomeArrayFullOfTelephonyIds();
var callLogs = new List<CallLogInfo>();
foreach (var callId in listOfCallIds) {
  var callLog = await rc.Restapi().Account("~").Extension("~").CallLog(callId).Get();
  callLogs.Add(callLog);
  if (rateLimitTracking["Heavy"] < 1) {
    // Wait some time to avoid getting rate limited
    await Task.Delay(1000 * 61);
  } 
}
tylerlong commented 4 years ago

I can add an experimental feature. Add rateLimitTracking to RestClient. So that as end user you don't need to parse headers.

tylerlong commented 4 years ago

New feature added in 4.0.0-beta3

Usage: https://github.com/ringcentral/RingCentral.Net/blob/master/RingCentral.Tests/RateLimitsTest.cs

tylerlong commented 3 years ago

New users should use the Rate Limit Extension