Better logging for backend components #3939

namkyu1999 · 2023-04-03T19:20:25Z

Proposed changes

Currently, our server logs provide limited information, making it difficult to diagnose issues and track user activity. So, I made Todos for better logging. Codes about custom log fields will be handled after LFX sync-up meeting and will be raised in the next pr.

Result

TODO

change default 'log' package to 'logrus' package
change name 'logrus' to 'log' for generality
log gRPC request by using middleware
log HTTP request by using Gin
support setting log format(JSON/text) at the start flag
add a custom field to logrus(will be addressed in next pr)

Issue No. #3932

Types of changes

What types of changes does your code introduce to Litmus? Put an x in the boxes that apply

New feature (non-breaking change which adds functionality)
Bugfix (non-breaking change which fixes an issue)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation Update (if none of the other choices applies)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

I have read the CONTRIBUTING doc
I have signed the commit for DCO to be passed.
Lint and unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have added necessary documentation (if appropriate)

Dependency

Please add the links to the dependent PR need to be merged before this (if any).

Special notes for your reviewer:

Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>

litmus-portal/graphql-server/pkg/handlers/request_logger.go

Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>

…tter-logging Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>

Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>

litmus-portal/graphql-server/pkg/rest_handlers/request_logger.go

imrajdas

@namkyu1999 - Few changes:

logs should be in small cases
Log field should be in camel cases eg, logrus.Withfield({workflowId: "123"})
Please use logrus.SetReportCaller(true)- this will report the line number in the log where the error occurred.

imrajdas · 2023-04-10T17:14:34Z

Logs should be printing in the base caller function or graphql resolver function. Otherwise the logs will be printed twice. From other function, we can return the error with required info,

return fmt.Errorf("failed due to ...., error: %v", err)

Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>

litmus-portal/graphql-server/pkg/rest_handlers/request_logger.go

Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>

namkyu1999 · 2023-04-11T05:10:44Z

Can you assign me, please? @imrajdas
And I resolved all changes you mentioned.

imrajdas

Please make the same changes in other areas also.

litmus-portal/graphql-server/graph/cluster.resolvers.go

litmus-portal/graphql-server/pkg/analytics/ops/prometheus/health-check.go

Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>

litmus-portal/graphql-server/graph/analytics.resolvers.go

S-ayanide · 2023-04-17T04:32:32Z

litmus-portal/graphql-server/graph/cluster.resolvers.go

@@ -149,7 +149,7 @@ func (r *subscriptionResolver) ClusterConnect(ctx context.Context, clusterInfo m

 		err = dbOperationsCluster.UpdateCluster(query, update)
 		if err != nil {
-			logrus.Print("Error", err)
+			log.Error(err)


Suggested change

log.Error(err)

log.Fatal(err)

S-ayanide · 2023-04-17T04:37:00Z

litmus-portal/graphql-server/graph/image_registry.resolvers.go

@@ -22,7 +22,7 @@ func (r *mutationResolver) CreateImageRegistry(ctx context.Context, projectID st

 	ciResponse, err := imageRegistryOps.CreateImageRegistry(ctx, projectID, imageRegistryInfo)
 	if err != nil {
-		logrus.Error(err)
+		log.Error(err)


Can you convert all

if err != nil

condition logs to log.Fatal?

Hey @S-ayanide,
log.Fatal called os.Exit(1) so that process will exit with status set to 1. Is it okay?

https://pkg.go.dev/github.com/sirupsen/logrus#Fatal

Yes that is the expectation, if something in the middle goes wrong we'd want to terminate that function with a Fatal message.

@amityt correct me if I'm wrong.

No log.Fatal is not required here. It will impact the complete server. Rather a return statement can be added here in this error block.

If we use log.Fatal() to exit the process, k8s Deployment obj restarts the pod automatically. IMO, It's an unnecessary task. Moreover, all of the logs are deleted so we cannot analyze what is the problem.

Yes, log.Fatal() should not be used here. 👍🏼

Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>

amityt

LGTM, requested some minor changes.

litmus-portal/graphql-server/graph/image_registry.resolvers.go

litmus-portal/graphql-server/pkg/gitops/gitops.go

Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>

S-ayanide

LGTM 🚀

litmus-portal/graphql-server/pkg/self-deployer/start.go

Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>

fix: change package name logrus to log for generality

639f32c

Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>

github-advanced-security bot found potential problems Apr 3, 2023

View reviewed changes

litmus-portal/graphql-server/pkg/handlers/request_logger.go Fixed Show fixed Hide fixed

namkyu1999 added 2 commits April 5, 2023 16:58

feat: add middleware in grpc for logging

6f1893f

Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>

feat: add feature that can select log format by env

fd09c66

Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>

namkyu1999 force-pushed the better-logging branch from b7aed50 to fd09c66 Compare April 7, 2023 06:23

namkyu1999 added 2 commits April 8, 2023 02:37

Merge branch 'master' of https://github.com/namkyu1999/litmus into be…

89596c3

…tter-logging Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>

feat: add request logger middleware

b313b89

Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>

github-advanced-security bot found potential problems Apr 7, 2023

View reviewed changes

litmus-portal/graphql-server/pkg/rest_handlers/request_logger.go Fixed Show fixed Hide fixed

namkyu1999 changed the title ~~Add better logging for backend components~~ Better logging for backend components Apr 7, 2023

namkyu1999 marked this pull request as ready for review April 7, 2023 18:14

imrajdas requested changes Apr 10, 2023

View reviewed changes

namkyu1999 added 2 commits April 11, 2023 11:57

fix: make logs as camel case and set logrus caller

18af03d

Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>

fix: fix log fields

e1daf6c

Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>

github-advanced-security bot found potential problems Apr 11, 2023

View reviewed changes

litmus-portal/graphql-server/pkg/rest_handlers/request_logger.go Fixed Show fixed Hide fixed

fix: fix codeql issue

5020b15

Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>

namkyu1999 requested a review from imrajdas April 11, 2023 05:10

imrajdas assigned namkyu1999 Apr 11, 2023

imrajdas reviewed Apr 12, 2023

View reviewed changes

fix: optimizing logs

b13a59b

Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>

namkyu1999 requested a review from imrajdas April 12, 2023 12:02

S-ayanide added the LFX-MENTORSHIP Linux Foundation Mentor ship Issue label Apr 17, 2023

S-ayanide requested changes Apr 17, 2023

View reviewed changes

fix: replace upper to lower

fb1cbf4

Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>

amityt approved these changes Apr 18, 2023

View reviewed changes

fix: fix minor tasks

ad0f1da

Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>

namkyu1999 requested a review from S-ayanide April 18, 2023 04:59

amityt approved these changes Apr 18, 2023

View reviewed changes

S-ayanide approved these changes Apr 18, 2023

View reviewed changes

Saranya-jena approved these changes Apr 18, 2023

View reviewed changes

Merge branch 'master' into better-logging

5f187d9

Jonsy13 reviewed Apr 18, 2023

View reviewed changes

litmus-portal/graphql-server/pkg/self-deployer/start.go Outdated Show resolved Hide resolved

fix: fix log comments

a9a7a71

Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>

Jonsy13 approved these changes Apr 18, 2023

View reviewed changes

Jonsy13 merged commit 48379a4 into litmuschaos:master Apr 18, 2023
16 checks passed

S-ayanide mentioned this pull request Apr 24, 2023

Add better logging for backend components #3932

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better logging for backend components #3939

Better logging for backend components #3939

namkyu1999 commented Apr 3, 2023 •

edited

imrajdas left a comment •

edited

imrajdas commented Apr 10, 2023

namkyu1999 commented Apr 11, 2023 •

edited

imrajdas left a comment

S-ayanide Apr 17, 2023

S-ayanide Apr 17, 2023

namkyu1999 Apr 17, 2023

S-ayanide Apr 17, 2023

amityt Apr 17, 2023

namkyu1999 Apr 17, 2023

amityt Apr 17, 2023

amityt left a comment

S-ayanide left a comment

Better logging for backend components #3939

Better logging for backend components #3939

Conversation

namkyu1999 commented Apr 3, 2023 • edited

Proposed changes

Types of changes

Checklist

Dependency

Special notes for your reviewer:

imrajdas left a comment • edited

Choose a reason for hiding this comment

imrajdas commented Apr 10, 2023

namkyu1999 commented Apr 11, 2023 • edited

imrajdas left a comment

Choose a reason for hiding this comment

S-ayanide Apr 17, 2023

Choose a reason for hiding this comment

S-ayanide Apr 17, 2023

Choose a reason for hiding this comment

namkyu1999 Apr 17, 2023

Choose a reason for hiding this comment

S-ayanide Apr 17, 2023

Choose a reason for hiding this comment

amityt Apr 17, 2023

Choose a reason for hiding this comment

namkyu1999 Apr 17, 2023

Choose a reason for hiding this comment

amityt Apr 17, 2023

Choose a reason for hiding this comment

amityt left a comment

Choose a reason for hiding this comment

S-ayanide left a comment

Choose a reason for hiding this comment

namkyu1999 commented Apr 3, 2023 •

edited

imrajdas left a comment •

edited

namkyu1999 commented Apr 11, 2023 •

edited