switch to structured logging #304
Conversation
|
Hi @pinikomarov. Thanks for your PR. I'm waiting for a openstack-k8s-operators 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. |
pinikomarov
force-pushed
the
structured_logging
branch
from
23bbc10
to
b976c38
Compare
pinikomarov
force-pushed
the
structured_logging
branch
2 times, most recently
from
e017048
to
dc23c0d
Compare
|
/ok-to-test |
|
/retest-required |
pinikomarov
force-pushed
the
structured_logging
branch
from
54836ff
to
a89a0bf
Compare
pinikomarov
force-pushed
the
structured_logging
branch
5 times, most recently
from
e82dc49
to
7590213
Compare
pinikomarov
force-pushed
the
structured_logging
branch
from
7590213
to
a9f2d45
Compare
pinikomarov
force-pushed
the
structured_logging
branch
from
a9f2d45
to
1462529
Compare
pinikomarov
force-pushed
the
structured_logging
branch
from
1462529
to
3fe31a0
Compare
|
@fmount are we good here ? can we proceed ? |
Checking the patch. |
controllers/glance_controller.go
Outdated
| Scheme *runtime.Scheme | ||
| } | ||
|
|
||
| // GetLogger returns a logger object with a logging prefix of "controller.name" and additional controller context fields | ||
| func (r *GlanceReconciler) GetLogger(ctx context.Context) logr.Logger { | ||
| return log.FromContext(ctx).WithName("Controllers").WithName("Glance") |
There was a problem hiding this comment.
what about having: return log.FromContext(ctx).WithName("Controllers").WithName(string(glance.Glance))
controllers/glanceapi_controller.go
Outdated
| Scheme *runtime.Scheme | ||
| } | ||
|
|
||
| // GetLogger returns a logger object with a logging prefix of "controller.name" and additional controller context fields | ||
| func (r *GlanceAPIReconciler) GetLogger(ctx context.Context) logr.Logger { | ||
| return log.FromContext(ctx).WithName("Controllers").WithName("GlanceAPI") |
There was a problem hiding this comment.
what about having: return log.FromContext(ctx).WithName("Controllers").WithName(string(glance.GlanceAPI))
| Scheme *runtime.Scheme | ||
| } | ||
|
|
||
| // GetLogger returns a logger object with a logging prefix of "controller.name" and additional controller context fields |
There was a problem hiding this comment.
Wondering if we can move the GetLogger function in glance_common.go and eventually pass (as input) the component.
This way we can extend the behavior and we don't have to define it twice, as well as we can make it independent from the caller. @pinikomarov can you check this?
There was a problem hiding this comment.
Basically add:
// GetLogger returns a logger object with a logging prefix of "controller.name" and additional controller context fields
func GetLogger(ctx context.Context, caller string) logr.Logger {
return log.FromContext(ctx).WithName("Controllers").WithName(caller)
}
to glance_common.go, and in both glance_controller.go and glanceapi_controller.go change the GetLogger call to something like:
// Reconcile reconcile Glance API requests
func (r *GlanceAPIReconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ctrl.Result, _err error) {
Log := GetLogger(ctx, string(glance.GlanceAPI))
...
...and
// Reconcile reconcile Glance requests
func (r *GlanceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ctrl.Result, _err error) {
Log := GetLogger(ctx, string(glance.Glance))
...
...By doing the above, you can define the logr.logger once and pass it to the underlying functions.
For instance, the reconcileDelete (but it applies to the other functions), can be changed to have in its signature the Log parameter:
func (r *GlanceReconciler) reconcileDelete(ctx context.Context, instance *glancev1.Glance, helper *helper.Helper, Log logr.Logger) (ctrl.Result, error) {
...
...and you don't have to call GetLogger every time, because it's already available and it has been instantiated in the main reconcile loop.
update controller str
pinikomarov
force-pushed
the
structured_logging
branch
from
3fe31a0
to
741f5f4
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pinikomarov 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 |
|
@pinikomarov: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
This automatically adds additional fields like reconcile_id etc.. from the controller context.
before :
2023-05-18T01:53:14+03:00 INFO controllers.KeystoneAPI Reconciled Service init successfullyafter:
2023-10-19T12:31:34+03:00 INFO Controllers.Glance Reconciling Service 'glance' init {"controller": "glance", "contr ollerGroup": "glance.openstack.org", "controllerKind": "Glance", "Glance": {"name":"glance","namespace":"openstack"}, "namespace": "ope nstack", "name": "glance", "reconcileID": "b00689f2-c505-49c4-a63e-7fb1f8d8cba2"}*by using per reconcile function respective logger objects, the intention is to lay the ground for parallel reconciliation in future and avoid race conditions as ctx objects are reconcile run specific.