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

feat: implementing klog structured logging #79

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: implementing klog structured logging #79

wants to merge 2 commits into from

Conversation

omerap12
Copy link
Member

@omerap12 omerap12 commented Sep 7, 2024 •
edited
Loading

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Switches the controller logging to use klog instead of the current controller-runtime logger.

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #13

Special notes for your reviewer:

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

@k8s-ci-robot k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Sep 7, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: omerap12
Once this PR has been reviewed and has the lgtm label, please assign enj for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 7, 2024
@omerap12
Copy link
Member Author

omerap12 commented Sep 7, 2024

cc @aramase

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 45.83333% with 13 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@f5b4d2a). Learn more about missing BASE report.

Files with missing lines Patch % Lines
internal/controller/secretsync_controller.go 53.84% 6 Missing ⚠️
pkg/provider/provider_client.go 20.00% 4 Missing ⚠️
cmd/main.go 0.00% 1 Missing ⚠️
internal/controller/conditions.go 75.00% 1 Missing ⚠️
pkg/metrics/exporter.go 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main      #79   +/-   ##
=======================================
  Coverage        ?   56.47%           
=======================================
  Files           ?       12           
  Lines           ?     1043           
  Branches        ?        0           
=======================================
  Hits            ?      589           
  Misses          ?      410           
  Partials        ?       44

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

googs1025
googs1025 reviewed Sep 9, 2024
View reviewed changes
cmd/main.go Outdated Show resolved Hide resolved
@omerap12 omerap12 force-pushed the logger branch 2 times, most recently from 2a18c55 to b894f68 Compare September 9, 2024 08:27
@AlexanderStocks
Copy link
Contributor

AlexanderStocks commented Sep 17, 2024 •
edited
Loading

When running these local tests, before installing the SSSC helm chart did you uncomment the provider-e2e-installer provider container? If not, I would expect to see those error logs.

@nilekhc
Copy link
Contributor

nilekhc commented Sep 23, 2024

/retitle feat: implementing klog for structured logging

@k8s-ci-robot k8s-ci-robot changed the title Using klog for structured logging feat: implementing klog for structured logging Sep 23, 2024
@nilekhc
Copy link
Contributor

nilekhc commented Sep 23, 2024

/retitle feat: implementing klog structured logging

@k8s-ci-robot k8s-ci-robot changed the title feat: implementing klog for structured logging feat: implementing klog structured logging Sep 23, 2024
nilekhc
nilekhc requested changes Sep 23, 2024
View reviewed changes
Copy link
Contributor

@nilekhc nilekhc left a comment

Choose a reason for hiding this comment

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

Added few comments. PTAL.

cmd/main.go
Development: true,
}
opts.BindFlags(flag.CommandLine)
klog.InitFlags(nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

wire up mlog here too. Ref - https://github.com/kubernetes-sigs/secrets-store-csi-driver/blob/981b21afa7f331860252f7b061cebe6e52eb22d3/cmd/secrets-store-csi-driver/main.go#L96-L104

cmd/main.go
flag.Parse()

ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts)))
Copy link
Contributor

Choose a reason for hiding this comment

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

all setupLog instances here should be replaced to use klog

internal/controller/conditions.go Outdated
if ss.Status.Conditions == nil {
ss.Status.Conditions = []metav1.Condition{}
}

if len(oldConditionType) > 0 {
logger.V(10).Info("Removing old condition", "oldConditionType", oldConditionType)
klog.V(10).InfoS("Removing old condition", "oldConditionType", oldConditionType)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably too high and will never get caught up in logs. Usually default log level is 2. Let's get this at V(4)

internal/controller/conditions.go Outdated
@@ -164,16 +162,16 @@ func (r *SecretSyncReconciler) updateStatusConditions(ctx context.Context, ss *s
condition.Message = ConditionMessageUnknown
}

logger.V(10).Info("Adding new condition", "newConditionType", newConditionType, "conditionReason", conditionReason)
klog.V(10).InfoS("Adding new condition", "newConditionType", newConditionType, "conditionReason", conditionReason)
Copy link
Contributor

Choose a reason for hiding this comment

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

V(4)

internal/controller/conditions.go Outdated
}

logger.V(10).Info("Updated status", "condition", condition)
klog.V(10).InfoS("Updated status", "condition", condition)
Copy link
Contributor

Choose a reason for hiding this comment

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

V(4)

pkg/provider/provider_client.go
@@ -180,7 +180,7 @@ func (p *PluginClientBuilder) Cleanup() {

for k := range p.conns {
if err := p.conns[k].Close(); err != nil {
klog.ErrorS(err, "error shutting down provider connection", "provider", k)
klog.ErrorS(err, "Error shutting down provider connection", "provider", k)
Copy link
Contributor

Choose a reason for hiding this comment

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

lower case

pkg/provider/provider_client.go
@@ -208,10 +208,10 @@ func (p *PluginClientBuilder) HealthCheck(ctx context.Context, interval time.Dur

runtimeVersion, err := Version(c, client)
if err != nil {
klog.V(5).ErrorS(err, "provider healthcheck failed", "provider", provider)
klog.V(5).ErrorS(err, "Provider healthcheck failed", "provider", provider)
Copy link
Contributor

Choose a reason for hiding this comment

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

V(4) and lower case

pkg/provider/provider_client.go
continue
}
klog.V(5).InfoS("provider healthcheck successful", "provider", provider, "runtimeVersion", runtimeVersion)
klog.V(5).InfoS("Provider healthcheck successful", "provider", provider, "runtimeVersion", runtimeVersion)
Copy link
Contributor

Choose a reason for hiding this comment

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

V(4) and lower case

pkg/provider/provider_client.go
@@ -248,7 +248,7 @@ func MountContent(ctx context.Context, client v1alpha1.CSIDriverProviderClient,
}
return nil, nil, err
}
klog.V(5).Info("finished mount request")
klog.V(5).InfoS("Finished mount request")
Copy link
Contributor

Choose a reason for hiding this comment

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

V(4) and lower case

pkg/provider/provider_client.go
@@ -266,7 +266,7 @@ func MountContent(ctx context.Context, client v1alpha1.CSIDriverProviderClient,
// Individual k8s secrets are limited to 1MiB in size.
// Ref: https://kubernetes.io/docs/concepts/configuration/secret/#restrictions
if size := proto.Size(resp); size > 1048576 {
klog.InfoS("proto above 1MiB, secret sync may fail", "size", size)
klog.InfoS("Proto above 1MiB, secret sync may fail", "size", size)
Copy link
Contributor

Choose a reason for hiding this comment

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

revert this to lower case

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 24, 2024
@omerap12
Fixed lint
2a98737 
Signed-off-by: Omer Aplatony <[email protected]>
@omerap12
Copy link
Member Author

omerap12 commented Oct 8, 2024

/retest-required

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

@googs1025 googs1025 googs1025 left review comments

@nilekhc nilekhc nilekhc requested changes

@enj enj Awaiting requested review from enj

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Finalize on the logger used for the controller
6 participants
@omerap12 @k8s-ci-robot @codecov-commenter @AlexanderStocks @nilekhc @googs1025