🌱Structured logging migration of cluster-api-provider-openstack/pkg/cloud/services/compute/instance.go #1617
Conversation
k8s-ci-robot
added
the
do-not-merge/hold
|
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: skylerxhu 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 |
k8s-ci-robot
added
the
cncf-cla: no
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Welcome @skylerxhu! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @skylerxhu. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
k8s-ci-robot
added
the
size/XS
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| @@ -94,7 +94,7 @@ func (s *Service) normalizePortTarget(port *infrav1.PortOpts, openStackCluster * | |||
| if err != nil { | |||
| // Multiple matches might be ok later when we restrict matches to a single network | |||
| if errors.Is(err, networking.ErrMultipleMatches) { | |||
| s.scope.Logger().V(4).Info("Can't infer network from subnet %d: %s", i, err) | |||
| s.scope.Logger().V(4).Info("Couldn't infer network from subnet","IPIndex", i, "err", err) | |||
There was a problem hiding this comment.
I would probably change IPIndex to SubnetIndex: FixedIPs is a weird naming thing from OpenStack which really means subnets.
However, both this and the above are very much debug level logs, so anybody interested is likely also looking at the code.
| @@ -432,7 +432,7 @@ func (s *Service) getOrCreateRootVolume(eventObject runtime.Object, instanceSpec | |||
| return nil, fmt.Errorf("exected to find volume %s with size %d; found size %d", name, size, volume.Size) | |||
| } | |||
|
|
|||
| s.scope.Logger().Info("using existing root volume %s", name) | |||
| s.scope.Logger().Info("Using existing root volume", "rootVolumeName", name) | |||
There was a problem hiding this comment.
This is an interesting rabbit hole. You've used rootVolumeName here, but volumeName below. I don't think this was covered in the guidelines, but I feel like these should be consistent.
But: what's the scope of the consistency? We could call this name and in the message below also change volumeID to ID. That would be consistent within the scope of a single message. But how about consistency across all log messages. How about if we always referred to the ID of a volume as volumeID. That sounds pretty useful to me when grepping for log messages. But I suspect (without checking) that in other places we just use ID for, e.g. a server ID. Should we update those to be serverID. Should we start a new taxonomy for log parameters:
<object>|(ID|Name|Idx)
Where <object> is one of:
- server
- volume
- loadBalancer
- port
- router
- network
- subnet
floatingIP would be slightly different, as they're normally referenced by IP.
Thinking about this now as we're starting a concerted effort to clean them all up, so we do have the opportunity to make everything consistent if we think it's worthwhile. If we agree on something like this we should add it to our documentation.
Interested in thoughts from @jichenjc @lentzi90 @seanschneeweiss @tobiasgiese @stephenfin.
There was a problem hiding this comment.
Good point. As you say, when we're doing this already it makes sense to be consistent in how we log.
There was a problem hiding this comment.
Perhaps it might make sense to add object types like "pool" and "listener" to the set. They are used extensively in pkg/cloud/services/loadbalancer/loadbalancer.go, and possibly other files, too.
There was a problem hiding this comment.
I agree that creating a new taxonomy will help address the consistency issue. During the migration to structured logging, I was confused by these two rules:
- "Do not provide additional context for how the value is used. Don't use
podIP, do useIP." - "When names are very ambiguous, try to include context in the label. For example, instead of
keyusecacheKeyor instead ofversionusedockerVersion."
There seems to be no clear instruction about when to include or exclude the context. This lack of clarity can be problematic for contributors who are not familiar with the default/conventional naming style. It would be beneficial to provide more explicit examples and explanations for each rule.
There was a problem hiding this comment.
@skylerxhu That's a good catch. That text from the kubernetes guidelines would appear to argue against what I'm suggesting. I think we should assume for now that the guidelines are based on collected experience and just follow them until we find a good reason not to.
To be clear, that means:
- A property (e.g. id, name, ip) for any object is just called <property>
- Unless the log message involves more than one object, in which case any 'principal subject' is just %lt;property> and other objects have a prefix
e.g.
("Created network", "id", "aaaa-bbbb-cccc-dddd", "name", "my-network")
("Created subnet", "id", "0000-1111-2222-3333", "name" "my-subnet", "networkID", "aaaa-bbbb-cccc-dddd", "networkName", my-network")
Thoughts?
There was a problem hiding this comment.
I don't think this is worth getting hung up on and it's impacting work by others, so lets aim to settle on a decision before EOD today.
There was a problem hiding this comment.
Also in favor of name if there is only one object handled within the log message.
There was a problem hiding this comment.
On the other hand, there is a list of "good keys", and that includes volumeName.
There was a problem hiding this comment.
By the way, can we use past tense here? "Used existing root volume"
Does that make sense?
There was a problem hiding this comment.
By the way, can we use past tense here? "Used existing root volume" Does that make sense?
IIRC the guidelines recommend '-ing' when logging before and action. Past tense indicates a completed action.
|
|
|
|
|
|
|
|
|
|
k8s-ci-robot
removed
the
size/XS
k8s-ci-robot
added
the
size/XL
|
@skylerxhu: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
| @@ -579,7 +579,7 @@ func (s *Service) DeleteInstance(eventObject runtime.Object, instanceStatus *Ins | |||
| return nil | |||
| } | |||
|
|
|||
| s.scope.Logger().Info("deleting dangling root volume %s(%s)", volume.Name, volume.ID) | |||
| s.scope.Logger().Info("Deleting dangling root volume", "volumeName", volume.Name, "volumeID", volume.ID) | |||
There was a problem hiding this comment.
I'm wondering if the logging should be moved out of this function to the calling function. And then it should log a line after calling getOrCreateRootVolume, using past tense.
E.g. "deleted volume".
This is something for a separate PR, however requires us to rethink where we log and where not.
What this PR does / why we need it:
Fix up the string formatting log messages so that they use structured logging as documented here: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md
/hold