Skip to content
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

fix(edge): parse the response body with failed request [EE-7244] #661

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

oscarzhou-portainer
Copy link
Collaborator

closes EE-7244

@oscarzhou-portainer oscarzhou-portainer force-pushed the fix/EE-7244/polling-api-log branch from 6ff46bc to 9426755 Compare July 8, 2024 05:03
Comment on lines -54 to -57
func newNonOkResponseError(msg string) *NonOkResponseError {
return &NonOkResponseError{msg: msg}
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot remove this, we rely on that error type in other parts of the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type was not removed, only the constructor function was removed, because I found that it is not used.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

agent/edge/poll.go

Lines 217 to 225 in 676d62a

environmentStatus, err := service.portainerClient.GetEnvironmentStatus()
if err != nil {
var nonOkError *client.NonOkResponseError
if errors.As(err, &nonOkError) {
service.edgeManager.SetEndpointID(0)
}
return err
}

}

return nil, errors.New("short poll request failed")
return nil, logPollingError(resp, "AsyncEdgeAgentExecuteAsyncRequest", fmt.Sprintf("AsyncEdgeAgent [%d] failed to execute async request", client.getEndpointIDFn()))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

err := json.NewDecoder(resp.Body).Decode(&errorData)
if err != nil {
log.Debug().CallerSkipFrame(1).
func logPollingError(resp *http.Response, ctxMsg, errMsg string) error {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should mix logging with error handling.

Str("error_response_details", errorData.Details).
Int("status_code", resp.StatusCode).
Msg("poll request failure")
log.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we use CallerSkipFrame() here as before?

Copy link
Collaborator

@andres-portainer andres-portainer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see the comments.

Copy link
Member

@deviantony deviantony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants