Remove usage of log.Fatal from httpclient.go; return an error object instead (FF-1934) #36
Conversation
…instead (FF-1934)
| if err != nil { | ||
| log.Fatal(err) | ||
| return "", err // Return an empty string and the error |
There was a problem hiding this comment.
According to the docs:
// An error is returned if caused by client policy (such as
// CheckRedirect), or failure to speak HTTP (such as a network
// connectivity problem). A non-2xx status code doesn't cause an
// error.
We should almost never expect to see this condition be executed.
| if resp.StatusCode == 401 { | ||
| hc.isUnauthorized = true | ||
| return "", fmt.Errorf("unauthorized access") // Return an error indicating unauthorized access | ||
| } | ||
|
|
||
| if resp.StatusCode >= 500 { | ||
| return "", fmt.Errorf("server error: %d", resp.StatusCode) // Handle server errors (status code > 500) |
There was a problem hiding this comment.
Another approach we can take here is to return "positive" when http is 2xx and have a generic error message otherwise.
| result, err := ecr.httpClient.get(RAC_ENDPOINT) | ||
| if err != nil { | ||
| fmt.Println("Failed to fetch RAC response", err) | ||
| return | ||
| } |
There was a problem hiding this comment.
I need to review canonical logging practice in go - I believe it would be better here to just log at the debug level.
There was a problem hiding this comment.
or perhaps warn level since its unexpected/unintended, but handled
There was a problem hiding this comment.
standard log package in Go does not directly support different logging levels like Warning. You would typically use log.Println or log.Printf for logging purposes and include a warning message manually. To accomplish this we will be using a third-party logging library that supports log levels, such as logrus, zap, or others.
I believe this is something we should move to (see comment from lucas below) to give our customers customization to use their own logger and I've added it to the scope of our upcoming debugging project.
| result, err := ecr.httpClient.get(RAC_ENDPOINT) | ||
| if err != nil { | ||
| fmt.Println("Failed to fetch RAC response", err) | ||
| return | ||
| } |
There was a problem hiding this comment.
or perhaps warn level since its unexpected/unintended, but handled
| "testing" | ||
| ) | ||
|
|
||
| func TestHttpClientGet(t *testing.T) { |
| result := ecr.httpClient.get(RAC_ENDPOINT) | ||
| result, err := ecr.httpClient.get(RAC_ENDPOINT) | ||
| if err != nil { | ||
| fmt.Println("Failed to fetch RAC response", err) |
There was a problem hiding this comment.
have you consider exposing a log in your sdk like this https://github.com/hyperjumptech/grule-rule-engine/blob/e4e90fe744ffffc1010edb7d12c5693a0b9ce477/logger/Logger.go#L79
like that you also extend the SDK for your clients to have their own log passed to your sdk and we can have better visibility.
There was a problem hiding this comment.
@lucasflink thank you for the suggestion and attached link - yes this is a great idea. we have an upcoming project starting imminently to improve the debug-ability of our SDKs and service for customers and I've included this in-scope.
I'm familiar with zap from working at Uber do you view as preferred? I will investigate whether a common interface (https://github.com/go-logr/logr) is possible.
There was a problem hiding this comment.
yes, most of the projects I worked with use the zap from Uber and I would also vow to use it. But maybe also consider creating your own interface with the common methods used for logging and like that any lib can be used and we would not be tight to only zap.
observation
When the Eppo API returns an HTTP >= 500, the SDK emits a
log.Fatal. I believe this was added without fully realizing its side-effect behavior of immediately exiting the program with status 1.In effect our
http_client.golibrary is deciding for the rest of the SDK what behavior anHTTP 500response should have. It is best to simply return this information upstream and let the relevant calling code make that determination, otherwise our server SDK customers will have regular process restarts.changes
log.Fatalerrorobject response togetmethod ofhttp_client.gohttp_client.goThe changes in SDK behavior are that the program will no longer exit on http 500; this case will simply log.