Skip to content

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.

…oute reconciler info logs, remove use of struct embedding in logs
@xWink xWink requested a review from mikhail-aws October 3, 2023 03:02
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

# 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
@xWink xWink deleted the logging-5 branch October 3, 2023 19:44
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


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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants