fix(edge): parse the response body with failed request [EE-7244]#661
fix(edge): parse the response body with failed request [EE-7244]#661oscarzhou-portainer wants to merge 8 commits intodevelopfrom
Conversation
oscarzhou-portainer
force-pushed
the
fix/EE-7244/polling-api-log
branch
from
6ff46bc to
9426755
Compare
| func newNonOkResponseError(msg string) *NonOkResponseError { | ||
| return &NonOkResponseError{msg: msg} | ||
| } | ||
|
|
There was a problem hiding this comment.
You cannot remove this, we rely on that error type in other parts of the code.
There was a problem hiding this comment.
The type was not removed, only the constructor function was removed, because I found that it is not used.
There was a problem hiding this comment.
That is technically true but the constructor function is not used anymore because you removed it in this PR. 🤣
You can't do that, we need to return that error because we depend on it in other places, please see:
Lines 217 to 225 in 676d62a
| } | ||
|
|
||
| return nil, errors.New("short poll request failed") | ||
| return nil, logPollingError(resp, "AsyncEdgeAgentExecuteAsyncRequest", fmt.Sprintf("AsyncEdgeAgent [%d] failed to execute async request", client.getEndpointIDFn())) |
There was a problem hiding this comment.
Please try to avoid using Sprintf() for this purpose, it's much better to use the structured logging functions that zlog provides, in this case something like .Int("endpoint_id", endpointID).
edge/client/error_response.go
Outdated
| err := json.NewDecoder(resp.Body).Decode(&errorData) | ||
| if err != nil { | ||
| log.Debug().CallerSkipFrame(1). | ||
| func logPollingError(resp *http.Response, ctxMsg, errMsg string) error { |
There was a problem hiding this comment.
I don't think we should mix logging with error handling.
edge/client/error_response.go
Outdated
| Str("error_response_details", errorData.Details). | ||
| Int("status_code", resp.StatusCode). | ||
| Msg("poll request failure") | ||
| log. |
There was a problem hiding this comment.
Shouldn't we use CallerSkipFrame() here as before?
andres-portainer
left a comment
andres-portainer
left a comment
There was a problem hiding this comment.
Please see the comments.
deviantony
left a comment
deviantony
left a comment
There was a problem hiding this comment.
I didn't had the time to do a full review yet but I just wanted to drop a quick comment regarding consistency across logging var names.
| log. | ||
| Error().Err(err.Err). | ||
| Str("context", ctxMsg). | ||
| Str("response message", err.Message). |
There was a problem hiding this comment.
| Str("response message", err.Message). | |
| Str("response_message", err.Message). |
We need to have a consistent naming for variables, I suggest that we use snake case for this. I will update the guidelines with that.
3 participants
closes EE-7244