Migrate assume_cache.go to structured logging #105904
Conversation
k8s-ci-robot
added
release-note
|
/wg structured-logging |
k8s-ci-robot
added
wg/structured-logging
|
@shivanshu1333: you can only set the release note label to release-note-none if the release-note block in the PR body text is empty or "none". In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
mengjiao-liu
force-pushed
the
structured_logging_scheduler
branch
from
8b92509
to
136bff5
Compare
mengjiao-liu
force-pushed
the
structured_logging_scheduler
branch
from
136bff5
to
b940fd7
Compare
|
/test pull-kubernetes-integration |
k8s-ci-robot
added
the
lgtm
shiva1333
moved this from Waiting on Reviewer
to Needs Approval
in WG Structured Logging - 1.23 Migration
|
ping @serathius |
mengjiao-liu
force-pushed
the
structured_logging_scheduler
branch
from
b940fd7
to
072de59
Compare
k8s-ci-robot
removed
the
lgtm
There was a problem hiding this comment.
When deciding on value key, context of how value is used (e.g. warning) is secondary to what value represents (e.g. error). Please don't pass Go Error types into other keys than "err".
Names of variables should not matter when deciding on logging keys. File assume_cache.go is example of bad naming. It's inconsistent and not even representing correct meaning of calue. Take for example "name", "objName" there are two different names for same thing used in different functions. We should definetly not repeat same pattern in logging. When users read logs they don't have easy access to code which would explain that those two keys have same values.
Another problem here that "name" is really bad variable name. You might think that this is object name but it's not. It's key to cache, you can confirm that by reading how this variable is generated "name, err := cache.MetaNamespaceKeyFunc(obj)" this is clearly a key for this cache and not an object name. I don't want to waste to much time but a proper fix would:
- Rename all variables "name" and "objName" to "key"
- Use "cacheKey" as a key in logging.
mengjiao-liu
force-pushed
the
structured_logging_scheduler
branch
from
072de59
to
f292a86
Compare
mengjiao-liu
force-pushed
the
structured_logging_scheduler
branch
from
f292a86
to
e262e3a
Compare
|
We could also consider renaming variables to avoid confusion, but this PR should be ok from migration side. |
k8s-ci-robot
added
the
lgtm
|
/assign @xing-yang For approval. |
|
/assign @gnufied |
|
This PR is needed to finish scheduler structured logging migration planned for 1.23 |
|
/assign @xing-yang for approval |
|
@shivanshu1333: GitHub didn't allow me to assign the following users: for, approval. Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
required for 1.23 |
|
@shivanshu1333: GitHub didn't allow me to assign the following users: approval, for. Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| return | ||
| } | ||
| } | ||
|
|
||
| objInfo := &objInfo{name: name, latestObj: obj, apiObj: obj} | ||
| if err = c.store.Update(objInfo); err != nil { | ||
| klog.Warningf("got error when updating stored object : %v", err) | ||
| klog.InfoS("Error occurred while updating stored object", "err", err) |
There was a problem hiding this comment.
Why does structured logging use Info to replace Warning msgs?
There was a problem hiding this comment.
As mentioned in the stuctured-logging-migration-guide, the concept behind using info is explained in logr, quoting verbatim for quick reference
Info logs are things you want to tell the user which are not errors. Error logs are, well, errors. If your code receives an error from a subordinate function call and is logging that error and not returning it, use error logs.
Verbosity-levels on info logs. This gives developers a chance to indicate arbitrary grades of importance for info logs, without assigning names with semantic meaning such as "warning", "trace", and "debug." Superficially this may feel very similar, but the primary difference is the lack of semantics. Because verbosity is a numerical value, it's safe to assume that an app running with higher verbosity means more (and less important) logs will be generated.
Hope, this helps :)
|
Just one question. Looks good other than that. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mengjiao-liu, xing-yang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Migrate assume_cache.go to structured logging
Which issue(s) this PR fixes:
Ref #105841
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: