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.

Sign up for GitHub

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

Jump to bottom

Logging Cleanup - Part 5 #419

Merged
merged 2 commits into from
Oct 3, 2023
Merged

Logging Cleanup - Part 5 #419

merged 2 commits into from
Oct 3, 2023

Conversation

xWink
Copy link
Member

@xWink xWink commented Oct 3, 2023

What type of PR is this?

Cleanup

Which issue does this PR fix:
#126 (final)

What does this PR do / Why do we need it:

  • Removes many unwanted info logs
  • Changes some error logs that do not actually indicate an error (e.g. expected errors that result in requeue) to debug
  • Removes struct embedding and use of ambiguous %v in logs
  • Add route reconciler info logs that match the other reconcilers
  • General logging language/clarity improvements
  • Added some missing error handling
  • Overall, running this with only info level logs should be way easier to follow along, and running with debug logs should be much less noisy without losing relevant/useful context

If an issue # is not available please add repro steps and logs from aws-gateway-controller showing the issue:

Testing done on this change:

Ran 32 of 32 Specs in 2058.765 seconds
SUCCESS! -- 32 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestIntegration (2059.73s)
PASS
ok      github.com/aws/aws-application-networking-k8s/test/suites/integration   2060.453s

Automation added to e2e:

Will this PR introduce any new dependencies?:

No

Will this break upgrades or downgrades. Has updating a running cluster been tested?:
No

Does this PR introduce any user-facing change?:

No, only logging changes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Change unwanted error logs to debug, remove unwanted info logs, add r…
9a4e9e7 
…oute reconciler info logs, remove use of struct embedding in logs
@xWink xWink requested a review from mikhail-aws October 3, 2023 03:02
zijun726911
zijun726911 approved these changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@zijun726911 zijun726911 left a comment

Choose a reason for hiding this comment

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

LGTM

Merge branch 'main' into logging-5
45612bd 
# Conflicts:
#	controllers/route_controller.go
#	controllers/serviceexport_controller.go
#	controllers/serviceimport_controller.go
#	pkg/deploy/lattice/target_group_synthesizer.go
#	pkg/gateway/model_build_lattice_service.go
#	pkg/gateway/model_build_rule.go
#	pkg/gateway/model_build_targetgroup.go
@xWink
Copy link
Member Author

xWink commented Oct 3, 2023

Reran e2e tests after resolving merge conflicts

Ran 32 of 32 Specs in 2135.827 seconds
SUCCESS! -- 32 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestIntegration (2136.74s)
PASS
ok      github.com/aws/aws-application-networking-k8s/test/suites/integration   2137.530s

@coveralls
Copy link

Pull Request Test Coverage Report for Build 6397658394

  • 30 of 66 (45.45%) changed or added relevant lines in 9 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.2%) to 45.77%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/deploy/lattice/listener_synthesizer.go 0 1 0.0%
pkg/gateway/model_build_lattice_service.go 0 1 0.0%
pkg/deploy/lattice/rule_synthesizer.go 0 2 0.0%
controllers/serviceexport_controller.go 0 4 0.0%
controllers/route_controller.go 0 10 0.0%
pkg/deploy/lattice/target_group_synthesizer.go 27 45 60.0%
Files with Coverage Reduction New Missed Lines %
pkg/deploy/lattice/target_group_synthesizer.go 1 80.66%
controllers/route_controller.go 2 0.0%
Totals Coverage Status
Change from base Build 6397211697: -0.2%
Covered Lines: 3825
Relevant Lines: 8357
💛 - Coveralls

@xWink xWink merged commit 658bb6e into aws:main Oct 3, 2023
5 checks passed
@xWink xWink deleted the logging-5 branch October 3, 2023 19:44
mikhail-aws
mikhail-aws reviewed Oct 3, 2023
View reviewed changes
pkg/gateway/model_build_targetgroup.go
@@ -94,7 +94,7 @@ func (t *svcExportTargetGroupModelBuildTask) run(ctx context.Context) error {

err = t.BuildTargets(ctx)
if err != nil {
t.log.Errorf("Failed to build targets for service export %s-%s due to %w",
t.log.Debugf("Failed to build targets for service export %s-%s due to %s",
Copy link
Contributor

Choose a reason for hiding this comment

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

should be error log

pkg/gateway/model_build_lattice_service.go
@@ -86,7 +86,7 @@ func (t *latticeServiceModelBuildTask) buildModel(ctx context.Context) error {
}

if err := t.buildTargetsForRoute(ctx); err != nil {
t.log.Errorf("failed to build targets due to %s", err)
t.log.Debugf("failed to build targets due to %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

should be error log

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikhail-aws mikhail-aws mikhail-aws left review comments

@zijun726911 zijun726911 zijun726911 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xWink @coveralls @zijun726911 @mikhail-aws