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

MG-2059 - Set correct log level depending on error #2196

Draft
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

nyagamunene
Copy link
Contributor

What type of PR is this?

This is a refactor because it sets correct log level depending on error.

What does this do?

It fixes the correct log level for the logs. Currently all log are warnings.

Which issue(s) does this PR fix/relate to?

Have you included tests for your changes?

No.

Did you document any new/modified feature?

No.

Notes

@nyagamunene nyagamunene force-pushed the MG2059-ErrorRefactoring branch 2 times, most recently from 533c151 to a596fd9 Compare April 29, 2024 07:23
@nyagamunene nyagamunene marked this pull request as ready for review April 29, 2024 07:23
Copy link
Contributor

@arvindh123 arvindh123 left a comment

Choose a reason for hiding this comment

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

Please check the following files, I found some error are logged as Warn :

auth/api/logging.go L30 to L128 https://github.com/nyagamunene/magistrala/blob/a596fd9abbf62be732b9185855def1f492f3baeb/auth/api/logging.go#L37-L129

bootstrap/api/logging.go

bootstrap/postgres/configs.go

certs/api/logging.go

cmd/opcua/main.go

coap/api/transport.go

consumers/notifiers/api/logging.go

consumers/writers/api/logging.go

internal/groups/api/logging.go

consumers/writers/api/logging.go

internal/groups/api/logging.go

lora/api/logging.go

invitations/middleware/logging.go

opcua/api/logging.go

pkg/events/redis/subscriber.go

things/api/logging.go

twins/api/logging.go

users/api/logging.go

ws/api/endpoints.go

Copy link
Member

@rodneyosodo rodneyosodo left a comment

Choose a reason for hiding this comment

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

Some errors should be logged with Warn while others should be logged with Error. This is a simple explanation of how to do it https://stackoverflow.com/a/2031195 . Also, consider using ErrorContext and pass in the context rather than Error

@nyagamunene nyagamunene force-pushed the MG2059-ErrorRefactoring branch 2 times, most recently from 95f66c4 to 7022dd0 Compare May 9, 2024 08:41
@dborovcanin dborovcanin force-pushed the MG2059-ErrorRefactoring branch from 7022dd0 to a8be062 Compare May 15, 2024 12:28
@@ -34,7 +34,7 @@ func (lm *loggingMiddleware) ListObjects(ctx context.Context, pr auth.PolicyReq,
}
if err != nil {
args = append(args, slog.Any("error", err))
lm.logger.Warn("List objects failed to complete successfully", args...)
lm.logger.ErrorContext(ctx, "List objects failed to complete successfully", args...)
Copy link
Member

Choose a reason for hiding this comment

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

Revert all errors to WarnContext

@@ -27,7 +27,7 @@ func handshake(ctx context.Context, svc ws.Service) http.HandlerFunc {
}
conn, err := upgrader.Upgrade(w, r, nil)
if err != nil {
logger.Warn(fmt.Sprintf("Failed to upgrade connection to websocket: %s", err.Error()))
logger.Error(fmt.Sprintf("Failed to upgrade connection to websocket: %s", err.Error()))
Copy link
Member

Choose a reason for hiding this comment

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

Use parent context

@@ -62,7 +62,7 @@ func decodeRequest(r *http.Request) (connReq, error) {

channelParts := channelPartRegExp.FindStringSubmatch(r.RequestURI)
if len(channelParts) < 2 {
logger.Warn("Empty channel id or malformed url")
logger.Error("Empty channel id or malformed url")
Copy link
Member

Choose a reason for hiding this comment

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

Use request context

@arvindh123 arvindh123 force-pushed the MG2059-ErrorRefactoring branch from a8be062 to bdcbd75 Compare May 20, 2024 13:27
@nyagamunene nyagamunene force-pushed the MG2059-ErrorRefactoring branch from bdcbd75 to 567b80a Compare May 20, 2024 22:26
@nyagamunene nyagamunene changed the title MG-2059-Set correct log level depending on error MG-2059 - Set correct log level depending on error May 21, 2024
@nyagamunene nyagamunene self-assigned this Jun 6, 2024
@nyagamunene nyagamunene marked this pull request as draft June 6, 2024 10:36
@nyagamunene nyagamunene force-pushed the MG2059-ErrorRefactoring branch from 8fb868f to ac0a377 Compare June 12, 2024 17:21
@nyagamunene nyagamunene force-pushed the MG2059-ErrorRefactoring branch from 7911b5b to 363c11d Compare June 12, 2024 17:35
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
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.

Set correct log level depending on error
4 participants