-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove usage of log.Fatal from httpclient.go; return an error object instead (FF-1934) #36
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,11 +40,15 @@ func (ecr *experimentConfigurationRequestor) GetConfiguration(experimentKey stri | |
} | ||
|
||
func (ecr *experimentConfigurationRequestor) FetchAndStoreConfigurations() { | ||
result := ecr.httpClient.get(RAC_ENDPOINT) | ||
result, err := ecr.httpClient.get(RAC_ENDPOINT) | ||
if err != nil { | ||
fmt.Println("Failed to fetch RAC response", err) | ||
return | ||
} | ||
Comment on lines
+43
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or perhaps There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
var wrapper racResponse | ||
|
||
// Unmarshal JSON data directly into the wrapper struct | ||
err := json.Unmarshal([]byte(result), &wrapper) | ||
err = json.Unmarshal([]byte(result), &wrapper) | ||
if err != nil { | ||
fmt.Println("Failed to unmarshal RAC response JSON", result) | ||
fmt.Println(err) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,8 @@ | ||
package eppoclient | ||
|
||
import ( | ||
"fmt" | ||
"io" | ||
"log" | ||
"time" | ||
|
||
"net/http" | ||
|
@@ -33,10 +33,13 @@ func newHttpClient(baseUrl string, client *http.Client, sdkParams SDKParams) *ht | |
return hc | ||
} | ||
|
||
func (hc *httpClient) get(resource string) string { | ||
func (hc *httpClient) get(resource string) (string, error) { | ||
url := hc.baseUrl + resource | ||
|
||
req, _ := http.NewRequest(http.MethodGet, url, nil) | ||
req, err := http.NewRequest(http.MethodGet, url, nil) | ||
if err != nil { | ||
return "", err // Return an empty string and the error | ||
} | ||
req.Header.Set("Content-Type", "application/json; charset=UTF-8") | ||
|
||
q := req.URL.Query() | ||
|
@@ -46,18 +49,31 @@ func (hc *httpClient) get(resource string) string { | |
req.URL.RawQuery = q.Encode() | ||
|
||
resp, err := hc.client.Do(req) | ||
|
||
if err != nil { | ||
log.Fatal(err) | ||
// from https://golang.org/pkg/net/http/#Client.Do | ||
// | ||
// 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. | ||
return "", err // Return an empty string and the error | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the docs:
We should almost never expect to see this condition be executed. |
||
} | ||
defer resp.Body.Close() // Ensure the response body is closed | ||
|
||
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) | ||
Comment on lines
65
to
+71
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another approach we can take here is to return "positive" when http is 2xx and have a generic error message otherwise. |
||
} | ||
|
||
b, err := io.ReadAll(resp.Body) | ||
if err != nil { | ||
log.Fatalln(err) | ||
return "", fmt.Errorf("server error: unreadable body") // Return an empty string and the error | ||
} | ||
return string(b) | ||
return string(b), nil // Return the response body as a string and nil for the error | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
package eppoclient | ||
|
||
import ( | ||
"net/http" | ||
"net/http/httptest" | ||
"testing" | ||
) | ||
|
||
func TestHttpClientGet(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🙌 |
||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
switch r.URL.Path { | ||
case "/test": | ||
w.WriteHeader(http.StatusOK) | ||
_, _ = w.Write([]byte(`OK`)) | ||
case "/unauthorized": | ||
w.WriteHeader(http.StatusUnauthorized) | ||
_, _ = w.Write([]byte(`Unauthorized`)) | ||
case "/internal-error": | ||
w.WriteHeader(http.StatusInternalServerError) | ||
_, _ = w.Write([]byte(`Internal Server Error`)) | ||
case "/bad-response": | ||
w.WriteHeader(http.StatusOK) | ||
if hijacker, ok := w.(http.Hijacker); ok { | ||
conn, _, _ := hijacker.Hijack() | ||
conn.Close() // Close the connection to simulate an unreadable body | ||
} | ||
} | ||
})) | ||
defer server.Close() | ||
|
||
client := &http.Client{} | ||
hc := newHttpClient(server.URL, client, SDKParams{ | ||
apiKey: "testApiKey", | ||
sdkName: "testSdkName", | ||
sdkVersion: "testSdkVersion", | ||
}) | ||
|
||
tests := []struct { | ||
name string | ||
resource string | ||
expectedError string | ||
expectedResult string | ||
}{ | ||
{ | ||
name: "api returns http 200", | ||
resource: "/test", | ||
expectedResult: "OK", | ||
}, | ||
{ | ||
name: "api returns 401 unauthorized error", | ||
resource: "/unauthorized", | ||
expectedError: "unauthorized access", | ||
}, | ||
{ | ||
name: "api returns an 500 error", | ||
resource: "/internal-error", | ||
expectedError: "server error: 500", | ||
}, | ||
{ | ||
name: "api returns unreadable body", | ||
resource: "/bad-response", | ||
expectedError: "server error: unreadable body", | ||
}, | ||
} | ||
|
||
for _, tc := range tests { | ||
t.Run(tc.name, func(t *testing.T) { | ||
result, err := hc.get(tc.resource) | ||
if err != nil { | ||
if err.Error() != tc.expectedError { | ||
t.Errorf("Expected error %v, got %v", tc.expectedError, err) | ||
} | ||
if result != "" { // Check if result is not an empty string when an error is expected | ||
t.Errorf("Expected result to be an empty string when there is an error, got %v", result) | ||
} | ||
} else { | ||
if tc.expectedError != "" { | ||
t.Errorf("Expected error %v, got nil", tc.expectedError) | ||
} | ||
if result != tc.expectedResult { | ||
t.Errorf("Expected result %v, got %v", tc.expectedResult, result) | ||
} | ||
} | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.