add more geoip service urls #2015
Conversation
Signed-off-by: mariobassem <mariobassem12@gmail.com>
MarioBassem
requested review from
muhamadazmy,
delandtj,
ashraffouda,
rawdaGastan and
xmonader
as code owners
pkg/geoip/geoip.go
Outdated
| return l, err | ||
| } | ||
| defer resp.Body.Close() | ||
| for i := 0; i < len(geoipURLs); i++ { |
pkg/geoip/geoip.go
Outdated
| log.Err(err).Msgf("failed to make http call to geoip service %s. retrying...", geoipURLs[i]) | ||
| continue | ||
| } | ||
| defer resp.Body.Close() |
There was a problem hiding this comment.
this won't be properly handled (you're calling the defer within a for-loop)
There was a problem hiding this comment.
I was expecting that we would fire up multiple requests in the background against the 3 geoip services and as soon one of the returns a healthy result we can use it, but @muhamadazmy can decide how to approach it properly
Signed-off-by: mariobassem <mariobassem12@gmail.com>
pkg/geoip/geoip.go
Outdated
| @@ -18,6 +20,22 @@ type Location struct { | |||
|
|
|||
| // Fetch retrieves the location of the system calling this function | |||
| func Fetch() (Location, error) { | |||
| geoipURLs := []string{"https://geoip.grid.tf/", "https://02.geoip.grid.tf/", "https://03.geoip.grid.tf/"} | |||
There was a problem hiding this comment.
can this be defined in the global scope (outside the function body) not on the stack. it's more of a constant than a variable. like
var (
geoipURLs = []string{...}
)
pkg/geoip/geoip.go
Outdated
| for _, url := range geoipURLs { | ||
| l, err := getLocation(url) | ||
| if err != nil { | ||
| log.Err(err).Msgf("failed to fetch location from geoip service %s. retrying...", url) |
There was a problem hiding this comment.
the log here is kinda misleading, we are not retrying instead we are trying a completely different url. Instead the error should show which url is not working + the cause. Also please use the structured logs correctly. Use the available, Str, Int, etc.. not formatting as much as possiblel
log.Err(err).Str("url", url).Msg("failed to fetch location from geoip service")Is more correct form than the one in code
Signed-off-by: mariobassem <mariobassem12@gmail.com>
Related issues: