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.

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

MGMT-6455 KubeAPI Controllers Debuggability: Add Logger Fields #1741

Merged
merged 1 commit into from
May 25, 2021

Conversation

nmagnezi
Copy link
Contributor

@nmagnezi nmagnezi commented May 17, 2021

This change adds logger fields to controllers to specify the
resource name and namespace that is being reconciled.

Additionally, in a case that context does not have a request-id,
we'll generate one to track it down both in controllers and the backend.

Lastly, I added a log indicating that a reconcile iteration has ended.

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 17, 2021
@openshift-ci openshift-ci bot requested review from empovit and gamli75 May 17, 2021 18:49
@nmagnezi nmagnezi force-pushed the ctrl_logger_fields branch from c53ef3d to 4af778f Compare May 17, 2021 19:34
@nmagnezi
Copy link
Contributor Author

/test subsystem-kubeapi-aws

@nmagnezi
Copy link
Contributor Author

/test subsystem-aws

@YuviGold
Copy link
Contributor

Could we change the notation to something else than spaces?
Instead of

		logrus.Fields{
			"agent":                       agent.Name,
			"agent namespace":             agent.Namespace,
			"clusterDeployment":           clusterDeployment.Name,
			"clusterDeployment namespace": clusterDeployment.Namespace,
		})

Maybe clusterDeployment_namespace or clusterDeployment-namespace
cc @filanov

@filanov
Copy link
Contributor

filanov commented May 23, 2021

Could we change the notation to something else than spaces?
Instead of

		logrus.Fields{
			"agent":                       agent.Name,
			"agent namespace":             agent.Namespace,
			"clusterDeployment":           clusterDeployment.Name,
			"clusterDeployment namespace": clusterDeployment.Namespace,
		})

Maybe clusterDeployment_namespace or clusterDeployment-namespace
cc @filanov

Usually we call the fields with a snakecase for example request_id

@nmagnezi nmagnezi force-pushed the ctrl_logger_fields branch 2 times, most recently from 89b3cc8 to af7b00f Compare May 23, 2021 16:08
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2021
@nmagnezi nmagnezi changed the title NO-ISSUE KubeAPI Controllers Debuggability: Add Logger Fields MGMT-6455 KubeAPI Controllers Debuggability: Add Logger Fields May 23, 2021
@nmagnezi nmagnezi force-pushed the ctrl_logger_fields branch from af7b00f to ef9cfee Compare May 23, 2021 16:15
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2021
@nmagnezi nmagnezi force-pushed the ctrl_logger_fields branch 5 times, most recently from 1455cbc to 229d16b Compare May 24, 2021 11:55
@nmagnezi
Copy link
Contributor Author

PR temporarily rebased on #1606 so I can preemptively resolve the git conflicts and make relevant modifications (added logs).
Once #1606 gets merged, I'll rebased on master.

@nmagnezi nmagnezi force-pushed the ctrl_logger_fields branch 3 times, most recently from ffbe4b4 to 019626f Compare May 24, 2021 13:01
@nmagnezi
Copy link
Contributor Author

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 24, 2021
@nmagnezi nmagnezi requested a review from filanov May 24, 2021 13:10
@nmagnezi
Copy link
Contributor Author

/assign @filanov
/assign @rollandf
/assign @danielerez
/assign @RazRegev
/assign @eliorerz
/assign @djzager --> I'm proposing a logger change to agentserviceconfig_controller

@nmagnezi
Copy link
Contributor Author

/assign @djzager

Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

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

I think it will be nice for us to all use the same logger. I'll save the 👍 for someone else but this looks good to me.

@rollandf
Copy link

/lgtm

@openshift-ci openshift-ci bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. lgtm Indicates that a PR is ready to be merged. labels May 25, 2021
This change adds logger fields to controllers to specify the
resource name and namespace that is being reconciled.

Additionally, in a case that context does not have a request-id,
we'll generate one to track it down both in controllers and the backend.

Lastly, I added a log indicating that a reconcile iteration has ended.
@nmagnezi nmagnezi force-pushed the ctrl_logger_fields branch from 019626f to 727f769 Compare May 25, 2021 06:45
@openshift-ci openshift-ci bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 25, 2021
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 25, 2021
@openshift-ci
Copy link

openshift-ci bot commented May 25, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: filanov, nmagnezi

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@nmagnezi
Copy link
Contributor Author

/test e2e-metal-assisted-operator-ztp

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 0404f38 into openshift:master May 25, 2021
@nmagnezi nmagnezi deleted the ctrl_logger_fields branch May 27, 2021 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants