Feature/retry and ratelimit

[roffe]

Adds retry logic in case of 429 as well as ratelimit with sane defaults for the Battle.net API configurable via optional opts on the client to not break backwards compatibility

usage example to configure custom, shown with default values:

also I snuck in a http.Client config with keep-alive, should increase performance for people who use it in backends and system service doing a lot of lookups in a row

package main

import (
	"context"
	"encoding/json"
	"fmt"
	"log"
	"os"
	"time"

	"github.com/FuzzyStatic/blizzard/v2"
	"github.com/avast/retry-go"
)

var (
	bnetID     string
	bnetSECRET string
)

func init() {
	bnetID = os.Getenv("BNET_ID")
	bnetSECRET = os.Getenv("BNET_SECRET")
	if bnetID == "" || bnetSECRET == "" {
		log.Fatal("missing BNET_ID or BNET_SECRET")
	}
}
func main() {
	blizz := blizzard.NewClient(
		bnetID,
		bnetSECRET,
		blizzard.EU,
		blizzard.EnUS,
		// This is the client default rate config, we allow 100 requests per second, and a burst budget of 10 requests
		blizzard.NewRateOpt(1*time.Second/100, 10),
		// This is the client default retry config
		blizzard.NewRetryOpt(
			retry.Attempts(3),
			retry.Delay(100*time.Millisecond),
			retry.DelayType(retry.BackOffDelay),
			retry.MaxJitter(0),
			retry.RetryIf(func(err error) bool {
				switch {
				case err.Error() == "429 Too Many Requests":
					return true // recoverable error, retry
				case err.Error() == "403 Forbidden":
					return false
				case err.Error() == "404 Not Found":
					return false
				default:
					return false // We cannot retry this away
				}
			}),
		),
	)

	mount, _, err := blizz.WoWMountIndex(context.TODO())
	if err != nil {
		log.Fatal(err)
	}

	out, err := json.MarshalIndent(mount, "", "  ")
	if err != nil {
		log.Fatal(err)
	}

	fmt.Println(string(out[:]))
}

[roffe]

I'd love your thoughts on this @FuzzyStatic. I was sitting and writing wrappers for blizzard client methods in my project and figured it would probably be of value to this lib

[FuzzyStatic]

@roffe I'm a little wary about adding new dependencies. This package aims to be simple enough for anyone to use.

That being said, if this package is going to add these new parameters, defaults should always be used unless otherwise specified by the end-user. To clean this up the NewClient function should ingest a new structure called Config that contains clientID, clientSecret string, region Region, locale Locale as well as options for the HTTP client. I don't want to manage the HTTP client for the user without clear comments as to what the defaults are and, if they are changed from the package defaults, good reasons why they may be changed.

For rate-limiting and retrying, these options should default to not being used unless there is clear user-defined input, or in the case of Blizzard's own rate limit, a documented default constant value.

If you can get this PR up to the above requirements, then I can look into merging this PR into the codebase. Does that seem fair?

[roffe]

To clean this up the NewClient function should ingest a new structure called Config that contains clientID, clientSecret string, region Region, locale Locale as well as options for the HTTP client

The reason to why I added a variadic input to the end of NewClient was to not break backwards compatibility

Do you mean the new way to create a client would look something like this?:

config := &blizzard.Config{
	ClientID: "my id",
	ClientSecret: "my secret",
	Region: blizzard.EU,
	Locale: blizzard.EnUS,
	HttpClient: &http.Client{
		// ... opts
	},
}
blizz := blizzzard.NewClient(config)

For rate-limiting and retrying, these options should default to not being used unless there is clear user-defined input, or in the case of Blizzard's own rate limit, a documented default constant value.

https://develop.battle.net/documentation/guides/game-data-apis

Throttling API clients are limited to 36,000 requests per hour at a rate of 100 requests per second. Exceeding the hourly quota results in slower service until traffic decreases. Exceeding the per-second limit results in a 429 error for the remainder of the second until the quota refreshes.

I picked 3 as a default value with a exponential back-off for the retry, so if it was 100ms first, it's 200 second and 400ms wait the 3rd time

I'll think about how to implement retry without using 3rd party libs, is x/retry still ok?

Edit 1: meant golang.org/x/time/rate not x/retry

[FuzzyStatic]

@roffe

config := &blizzard.Config{
	ClientID: "my id",
	ClientSecret: "my secret",
	Region: blizzard.EU,
	Locale: blizzard.EnUS,
	HttpClient: &http.Client{
		// ... opts
	},
}
blizz := blizzzard.NewClient(config)

Yes, this is what I meant. For the rate limiter, if there is a const value with the Blizzard documentation attached I would be ok with it.

As for the retry, I'd still prefer the retry default be no retry unless parameters are specified in the config. I don't feel it's necessary to create your own retry implementation, avast is fine.