BED-4602: Reduce Verbose Log Output #161
Conversation
WalkthroughRefactors logging across many Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cmd/list-group-owners.go (1)
109-109: Consider logging owner identifier for better debugging context.The log currently emits only
groupId, which identifies the group but provides no information about which specific owner was found. For consistency with other files in this PR (e.g.,list-function-app-role-assignments.gologs id, roleDefinitionId, principalId) and to aid debugging, consider also logging an owner-specific identifier.🔎 Suggested enhancement
-log.V(2).Info("found group owner", "groupId", groupOwner.GroupId) +log.V(2).Info("found group owner", "groupId", groupOwner.GroupId, "ownerId", groupOwner.Owner.Id)cmd/list-group-members.go (1)
119-119: Consider logging member identifier for better debugging context.Similar to the group owners case, this log emits only
groupIdwithout any member-specific identifier. To improve debugging capability and maintain consistency with the logging pattern in other files (which typically log multiple identifying fields), consider adding a member identifier.🔎 Suggested enhancement
-log.V(2).Info("found group member", "groupId", groupMember.GroupId) +log.V(2).Info("found group member", "groupId", groupMember.GroupId, "memberId", groupMember.Member.Id)cmd/list-storage-accounts.go (1)
105-105: Logging change achieves the PR objective.The change successfully reduces log verbosity by logging only
idandnameinstead of the entire storage account object.Optional: Consider consistent field access pattern
The code mixes explicit field access (
storageAccount.StorageAccount.Id) with promoted field access (storageAccount.Name). For consistency, consider using either both promoted or both explicit:-log.V(2).Info("found storage account", "id", storageAccount.StorageAccount.Id, "name", storageAccount.Name) +log.V(2).Info("found storage account", "id", storageAccount.Id, "name", storageAccount.Name)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
.gitignorecmd/list-app-owners.gocmd/list-app-role-assignments.gocmd/list-apps.gocmd/list-automation-account-role-assignments.gocmd/list-automation-accounts.gocmd/list-container-registries.gocmd/list-container-registry-role-assignments.gocmd/list-device-owners.gocmd/list-devices.gocmd/list-function-app-role-assignments.gocmd/list-function-apps.gocmd/list-group-members.gocmd/list-group-owners.gocmd/list-groups.gocmd/list-key-vault-role-assignments.gocmd/list-key-vaults.gocmd/list-logic-app-role-assignments.gocmd/list-logic-apps.gocmd/list-managed-cluster-role-assignments.gocmd/list-managed-clusters.gocmd/list-management-group-role-assignments.gocmd/list-management-groups.gocmd/list-resource-group-role-assignments.gocmd/list-resource-groups.gocmd/list-role-assignments.gocmd/list-role-eligibility-schedule-instance.gocmd/list-roles.gocmd/list-service-principal-owners.gocmd/list-service-principals.gocmd/list-storage-account-role-assignments.gocmd/list-storage-accounts.gocmd/list-storage-containers.gocmd/list-subscription-owners.gocmd/list-subscription-role-assignments.gocmd/list-subscription-user-access-admins.gocmd/list-subscriptions.gocmd/list-tenants.gocmd/list-users.gocmd/list-virtual-machine-role-assignments.gocmd/list-virtual-machines.gocmd/list-vm-scale-set-role-assignments.gocmd/list-vm-scale-sets.gocmd/list-web-app-role-assignments.gocmd/list-web-apps.go
🧰 Additional context used
🧬 Code graph analysis (8)
cmd/list-tenants.go (2)
constants/misc.go (1)
DisplayName(29-29)client/query/params.go (1)
TenantId(41-41)
cmd/list-resource-group-role-assignments.go (1)
models/azure/role_assignment.go (1)
RoleAssignment(31-43)
cmd/list-subscription-role-assignments.go (1)
models/azure/role_assignment.go (1)
RoleAssignment(31-43)
cmd/list-service-principals.go (1)
constants/misc.go (1)
DisplayName(29-29)
cmd/list-key-vault-role-assignments.go (1)
models/azure/role_assignment.go (1)
RoleAssignment(31-43)
cmd/list-virtual-machine-role-assignments.go (1)
models/azure/role_assignment.go (1)
RoleAssignment(31-43)
cmd/list-management-group-role-assignments.go (1)
models/azure/role_assignment.go (1)
RoleAssignment(31-43)
cmd/list-storage-accounts.go (2)
models/azure/storage_account.go (1)
StorageAccount(22-34)models/storage-account.go (1)
StorageAccount(22-28)
🔇 Additional comments (43)
.gitignore (1)
133-134: Appropriate addition to gitignore.The new pattern for
.idea/**/go.imports.xmlis a valid gitignore entry that correctly targets JetBrains IDE-generated Go imports metadata. The placement within the "Generated files" subsection aligns well with similar patterns in the file.cmd/list-management-groups.go (1)
74-74: LGTM! Logging improvement reduces verbosity effectively.The change successfully reduces log output by logging only the management group's ID and name instead of the entire object, which aligns perfectly with the PR objective to minimize verbose log size while maintaining useful diagnostic information.
cmd/list-roles.go (1)
73-73: Effective reduction of verbose log output.The selective field logging (id, displayName, isEnabled) successfully addresses the PR objective while retaining the most relevant information for debugging and traceability. The field selection is appropriate for role entities and follows the structured logging pattern consistently applied across other list commands in this PR.
cmd/list-subscription-user-access-admins.go (1)
91-91: LGTM! Effective reduction in log verbosity.The change successfully replaces full-object logging with targeted field logging. The selected fields (id, principalId, scope, roleId) provide sufficient identifiers for debugging while significantly reducing log size, which aligns perfectly with the PR objective.
cmd/list-storage-containers.go (1)
113-113: LGTM! Effective log verbosity reduction.The change from logging the entire
storageContainerobject to logging only the key fields (id,name,tenant) successfully achieves the PR objective of reducing verbose log output while retaining sufficient information for debugging and tracing.cmd/list-devices.go (1)
73-73: LGTM - Log verbosity reduction implemented correctly.The change successfully reduces verbose log output by logging only the
deviceIdfield instead of the entire device object, which aligns with the PR objectives. TheDeviceIdfield exists on the Device type and is appropriately used as a unique identifier for logging.cmd/list-role-assignments.go (1)
109-109: The concern about nil pointer dereference is not applicable here.
RoleDefinitionis a value type (UnifiedRoleDefinition), not a pointer. In Go, value types always have a zero-value when unmarshaled from JSON withomitempty, and cannot be nil. Accessing.DisplayNameon an emptyUnifiedRoleDefinitionstruct is safe and returns an empty string rather than causing a panic. No defensive check is needed.cmd/list-web-apps.go (1)
111-111: LGTM! Logging change reduces verbosity effectively.The change from logging the entire
webAppobject to logging justidandnamefields aligns perfectly with the PR objective. These two fields provide sufficient context for debugging while significantly reducing log size.cmd/list-key-vault-role-assignments.go (1)
109-109: LGTM! Well-structured logging change.The log statement emits both the role assignment ID and the key vault ID, providing clear context for debugging while reducing log verbosity. The structured field approach is clean and effective.
cmd/list-users.go (1)
87-87: LGTM! Comprehensive and well-balanced logging.The log statement captures three key fields—
id,type, andisEnabled—which provide excellent debugging context while achieving the PR's goal of reducing log size. The field selection is thoughtful and practical.cmd/list-function-app-role-assignments.go (1)
113-113: LGTM! Typo fix and excellent logging enhancement.This change accomplishes two improvements:
- Fixes the typo: "asignment" → "assignment"
- Adds comprehensive structured logging with four key fields (id, roleDefinitionId, principalId, scope)
The detailed field selection provides excellent debugging context while still achieving significant log size reduction compared to logging the entire object.
cmd/list-key-vaults.go (1)
108-108: LGTM! Comprehensive logging with good field selection.The log statement captures three key fields—
id,name, andtenantId—which provide strong debugging context across different dimensions (technical identifier, human-readable name, and organizational scope). Well-balanced approach to verbosity reduction.cmd/list-groups.go (1)
73-73: LGTM! Clean and effective logging change.The log statement emits both
idandname(DisplayName), providing a good balance between a technical identifier and a human-readable label. This is consistent with similar changes across the codebase and achieves the PR's verbosity reduction goal.cmd/list-subscription-owners.go (1)
91-91: LGTM! Logging refinement reduces verbosity effectively.The change from logging the entire
subscriptionOwnerobject to logging specific key fields (principalId,role,subscriptionId) achieves the PR's objective of reducing verbose log output while maintaining essential debugging information.cmd/list-subscriptions.go (1)
95-95: LGTM! Concise logging for subscription identification.Logging only the
SubscriptionIdinstead of the entire object significantly reduces log verbosity while providing sufficient information for tracking subscription enumeration.cmd/list-logic-apps.go (1)
114-114: LGTM! Appropriate field selection for Logic App logging.The change to log
idandnamefields instead of the entirelogicappobject provides sufficient debugging information while significantly reducing log size.cmd/list-app-role-assignments.go (1)
105-105: LGTM! Comprehensive logging for role assignments.The structured logging of
id,appRoleId, andnamefields provides excellent context for debugging app role assignments while avoiding the verbosity of logging the entire object.cmd/list-automation-account-role-assignments.go (1)
113-113: LGTM! Essential fields logged for automation account role assignments.Logging
idandroleDefinitionIdcaptures the most important identifiers for automation account role assignments while reducing log verbosity.cmd/list-tenants.go (1)
84-84: LGTM! Thorough tenant identification in logs.Logging
id,name, andtenantIdprovides complete context for tenant enumeration while maintaining concise log output.cmd/list-managed-cluster-role-assignments.go (1)
118-118: LGTM! Comprehensive logging for managed cluster role assignments.The structured logging of
id,roleDefinitionId, andprincipalIdprovides excellent debugging context for managed cluster role assignments while significantly reducing log verbosity.cmd/list-function-apps.go (1)
106-106: LGTM! Concise logging for Function Apps.Logging
idandnamefields provides sufficient information for tracking Function App enumeration while reducing log size, consistent with the pattern used across other list commands.cmd/list-vm-scale-set-role-assignments.go (1)
118-118: LGTM! Logging improvement reduces verbosity.The change from logging the entire
vmScaleSetRoleAssignmentobject to logging only the key fields (idandroleId) effectively reduces log volume while preserving essential debugging context.cmd/list-app-owners.go (1)
92-92: LGTM! Cleaner log output.Logging only the
appIdinstead of the entireappOwnerobject streamlines the verbose output while maintaining the essential identifier for debugging.cmd/list-resource-groups.go (1)
105-105: LGTM! Concise and informative logging.The change to log only
idandnamefields provides clear resource identification while significantly reducing log size compared to the full object dump.cmd/list-logic-app-role-assignments.go (1)
118-118: LGTM! Improved log clarity.Logging the specific
idandroleDefinitionIdfields instead of the entire role assignment object maintains debugging capability while reducing verbose output.cmd/list-managed-clusters.go (1)
109-109: LGTM! Streamlined logging.The change to log only
idandnameeffectively reduces log verbosity while retaining the essential identifiers for managed cluster debugging.cmd/list-virtual-machine-role-assignments.go (1)
109-109: LGTM! Targeted logging improvement.Logging only
roleIdandvirtualMachineIdprovides the necessary context for debugging virtual machine role assignments while significantly reducing log size.cmd/list-storage-account-role-assignments.go (1)
113-113: LGTM! Comprehensive yet concise logging.Logging
id,roleDefinitionId, andprincipalIdprovides sufficient context for storage account role assignment debugging while avoiding the overhead of logging the entire object.cmd/list-subscription-role-assignments.go (1)
109-109: LGTM! Efficient logging pattern.Logging only the nested
RoleAssignment.IdandRoleAssignment.Namefields effectively reduces log verbosity while preserving the key identifiers for subscription role assignment debugging.cmd/list-service-principal-owners.go (1)
109-109: LGTM! Logging refinement reduces verbosity effectively.The change from logging the entire
servicePrincipalOwnerobject to just theServicePrincipalIdfield reduces log size while preserving essential traceability for debugging.cmd/list-vm-scale-sets.go (1)
110-110: LGTM! Structured logging with key identifiers.Logging
idandnameinstead of the entire VM scale set object significantly reduces log volume while retaining the most relevant identifiers for troubleshooting.cmd/list-resource-group-role-assignments.go (1)
110-110: LGTM! Granular logging with essential role assignment details.The structured logging of
id,name, andresourceGroupIdprovides clear traceability for role assignments while dramatically reducing log size compared to serializing the entire object.cmd/list-service-principals.go (1)
73-73: LGTM! Logging refinement captures key service principal identifiers.Emitting
id,name(DisplayName), andappIdprovides the essential identifiers for service principals while avoiding the overhead of logging entire response objects.cmd/list-automation-accounts.go (1)
105-105: LGTM! Concise logging with key automation account identifiers.Logging
idandnameinstead of the full automation account object reduces log verbosity while maintaining adequate traceability for debugging.cmd/list-virtual-machines.go (1)
105-105: LGTM! Structured logging with multi-tenant context.Logging
id,name, andtenantIdprovides clear VM identification across tenants while significantly reducing log output compared to serializing the entire virtual machine object.cmd/list-container-registry-role-assignments.go (1)
118-118: LGTM! Comprehensive role assignment logging with key fields.The structured logging captures all essential role assignment attributes (
id,principalId,roleDefinitionId,scope) while avoiding the overhead of serializing entire objects—this provides excellent traceability for RBAC debugging.cmd/list-container-registries.go (1)
110-110: LGTM! Concise logging with key container registry identifiers.Logging
idandnameinstead of the full container registry object reduces log size while maintaining the essential identifiers needed for troubleshooting.cmd/list-web-app-role-assignments.go (1)
118-118: LGTM! Logging change reduces verbosity and fixes typo.The change from logging the entire
webAppRoleAssignmentstruct to just theroleDefinitionIdfield achieves the PR objective of reducing log output. Additionally, the typo fix ("asignment" → "assignment") improves code quality.cmd/list-apps.go (1)
73-73: LGTM! Logging change effectively reduces verbosity.The change from logging the entire application object to just the
idandnamefields aligns with the PR objective and significantly reduces log output while retaining essential diagnostic information.cmd/list-device-owners.go (1)
108-108: The current log statement is correct; no changes needed.The log change successfully reduces verbosity. The
Ownerfield inDeviceOwneris typed asjson.RawMessage(raw JSON data), not a structured object with accessible fields likeId. Logging only thedeviceIdis the appropriate approach here, as extracting owner information would require unmarshaling the JSON, which is not done at this point in the code.Likely an incorrect or invalid review comment.
cmd/list-role-eligibility-schedule-instance.go (2)
75-75: LGTM! Logging refactor aligns with PR objectives.The change successfully reduces log verbosity by logging only the essential identifying fields (id, roleDefinitionId, principalId) instead of the entire object. This will significantly reduce log size while maintaining useful debugging information.
The fields chosen are appropriate as they represent the primary identifiers for role eligibility schedule instances and match the fields used in the data structure below (lines 81-83).
22-24: No action needed—these imports are correct. The file is a new addition to the codebase, so all imports are necessarily new. Theos,os/signal, andtimeimports are all used appropriately in the code (signal handling and timing operations).cmd/list-management-group-role-assignments.go (1)
109-109: Log statement refactored to emit specific fields instead of full objects.The change replaces object-level logging with granular field-level logging (id, roleId, principalId, scope). All accessed fields are confirmed to exist:
RoleAssignment.Id,RoleAssignment.Properties.RoleDefinitionId,RoleAssignment.Properties.PrincipalId, andRoleAssignment.Properties.Scope. The verbosity level V(2) is appropriate for detailed logging.
StranDutton
force-pushed
the
BED-4602-modify-verbose-log-output
branch
from
86ea416 to
ef18db4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cmd/list-subscription-user-access-admins.go (1)
91-91: LGTM! Logging change successfully reduces verbosity.The modification correctly logs only key fields (id, principalId, scope, roleId) instead of the entire object, achieving the PR's objective. The field selection is appropriate for tracing subscription user access admins.
Optional: Consider line length for readability.
The log statement spans over 240 characters. If preferred, you could split this across multiple lines or extract fields into variables for improved readability, though this is purely a stylistic preference.
🔎 Optional refactor for readability
-log.V(2).Info("found subscription user access admin", "id", subscriptionUserAccessAdmin.UserAccessAdmin.Id, "principalId", subscriptionUserAccessAdmin.UserAccessAdmin.Properties.PrincipalId, "scope", subscriptionUserAccessAdmin.UserAccessAdmin.Properties.Scope, "roleId", subscriptionUserAccessAdmin.UserAccessAdmin.Properties.RoleDefinitionId) +admin := subscriptionUserAccessAdmin.UserAccessAdmin +log.V(2).Info("found subscription user access admin", + "id", admin.Id, + "principalId", admin.Properties.PrincipalId, + "scope", admin.Properties.Scope, + "roleId", admin.Properties.RoleDefinitionId)cmd/list-key-vault-role-assignments.go (1)
109-109: Consider adding principalId and roleDefinitionId to the log output.While the current fields identify the assignment and vault, including
principalId(who is assigned) androleDefinitionId(what role) would provide more actionable context when reviewing logs, especially since the PR description mentions principalId as an example field to include.🔎 Proposed enhancement
-log.V(2).Info("found key vault role assignment", "id", keyVaultRoleAssignment.RoleAssignment.Id, "keyVaultId", keyVaultRoleAssignment.KeyVaultId) +log.V(2).Info("found key vault role assignment", "id", keyVaultRoleAssignment.RoleAssignment.Id, "keyVaultId", keyVaultRoleAssignment.KeyVaultId, "principalId", keyVaultRoleAssignment.RoleAssignment.Properties.PrincipalId, "roleDefinitionId", keyVaultRoleAssignment.RoleAssignment.Properties.RoleDefinitionId)cmd/list-role-eligibility-schedule-instance.go (1)
75-75: LGTM! Logging change successfully reduces verbosity.The refactored log statement now emits only key identifiers (
id,roleDefinitionId,principalId) instead of the full object, which aligns perfectly with the PR objective to reduce log volume while maintaining traceability.Optionally, consider including
DirectoryScopeIdfor additional context, as it represents the scope of the role assignment and could be valuable for debugging permission-related issues:💡 Optional enhancement
- log.V(2).Info("found unified role eligibility instance schedule", "id", item.Ok.Id, "roleDefinitionId", item.Ok.RoleDefinitionId, "principalId", item.Ok.PrincipalId) + log.V(2).Info("found unified role eligibility instance schedule", "id", item.Ok.Id, "roleDefinitionId", item.Ok.RoleDefinitionId, "principalId", item.Ok.PrincipalId, "directoryScopeId", item.Ok.DirectoryScopeId)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
.gitignorecmd/list-app-owners.gocmd/list-app-role-assignments.gocmd/list-apps.gocmd/list-automation-account-role-assignments.gocmd/list-automation-accounts.gocmd/list-container-registries.gocmd/list-container-registry-role-assignments.gocmd/list-device-owners.gocmd/list-devices.gocmd/list-function-app-role-assignments.gocmd/list-function-apps.gocmd/list-group-members.gocmd/list-group-owners.gocmd/list-groups.gocmd/list-key-vault-role-assignments.gocmd/list-key-vaults.gocmd/list-logic-app-role-assignments.gocmd/list-logic-apps.gocmd/list-managed-cluster-role-assignments.gocmd/list-managed-clusters.gocmd/list-management-group-role-assignments.gocmd/list-management-groups.gocmd/list-resource-group-role-assignments.gocmd/list-resource-groups.gocmd/list-role-assignments.gocmd/list-role-eligibility-schedule-instance.gocmd/list-roles.gocmd/list-service-principal-owners.gocmd/list-service-principals.gocmd/list-storage-account-role-assignments.gocmd/list-storage-accounts.gocmd/list-storage-containers.gocmd/list-subscription-owners.gocmd/list-subscription-role-assignments.gocmd/list-subscription-user-access-admins.gocmd/list-subscriptions.gocmd/list-tenants.gocmd/list-users.gocmd/list-virtual-machine-role-assignments.gocmd/list-virtual-machines.gocmd/list-vm-scale-set-role-assignments.gocmd/list-vm-scale-sets.gocmd/list-web-app-role-assignments.gocmd/list-web-apps.go
✅ Files skipped from review due to trivial changes (1)
- cmd/list-web-apps.go
🚧 Files skipped from review as they are similar to previous changes (27)
- cmd/list-devices.go
- cmd/list-app-owners.go
- cmd/list-function-apps.go
- cmd/list-subscription-role-assignments.go
- cmd/list-storage-account-role-assignments.go
- cmd/list-resource-group-role-assignments.go
- cmd/list-service-principals.go
- cmd/list-group-owners.go
- cmd/list-managed-cluster-role-assignments.go
- cmd/list-vm-scale-sets.go
- cmd/list-web-app-role-assignments.go
- cmd/list-tenants.go
- cmd/list-device-owners.go
- cmd/list-virtual-machine-role-assignments.go
- cmd/list-storage-accounts.go
- cmd/list-container-registries.go
- cmd/list-virtual-machines.go
- cmd/list-automation-accounts.go
- cmd/list-service-principal-owners.go
- cmd/list-managed-clusters.go
- cmd/list-group-members.go
- cmd/list-users.go
- cmd/list-function-app-role-assignments.go
- cmd/list-groups.go
- cmd/list-logic-app-role-assignments.go
- cmd/list-container-registry-role-assignments.go
- cmd/list-vm-scale-set-role-assignments.go
🧰 Additional context used
🧬 Code graph analysis (3)
cmd/list-key-vault-role-assignments.go (1)
models/azure/role_assignment.go (1)
RoleAssignment(31-43)
cmd/list-management-group-role-assignments.go (1)
models/azure/role_assignment.go (1)
RoleAssignment(31-43)
cmd/list-apps.go (1)
constants/misc.go (1)
DisplayName(29-29)
🔇 Additional comments (15)
.gitignore (1)
133-134: LGTM!The addition of
.idea/**/go.imports.xmlis appropriately placed in the JetBrains section and follows the existing glob pattern convention. This prevents JetBrains IDE plugin metadata from being tracked, reducing unnecessary artifacts in version control.cmd/list-resource-groups.go (1)
105-105: LGTM! Logging improvement aligns with PR objectives.The change from logging the entire
resourceGroupobject to logging only theIdandNamefields effectively reduces log verbosity while preserving essential identifying information. This is sufficient for debugging and tracing enumerated resource groups, and the full object still flows through the pipeline for data processing.cmd/list-management-groups.go (1)
74-74: LGTM! Logging refinement aligns with PR objective.The change from logging the entire
item.Okobject to logging onlyidandnamefields effectively reduces verbose log output while preserving key identifying information needed for debugging.cmd/list-subscriptions.go (1)
95-95: LGTM! Improved logging granularity.The change successfully reduces log verbosity by logging only the subscription ID instead of the entire subscription object, while maintaining the same information value for troubleshooting.
cmd/list-apps.go (1)
73-73: LGTM! Enhanced structured logging.The change provides better structured logging by emitting individual fields (id and name) instead of the entire application object, making logs more readable and significantly smaller.
cmd/list-automation-account-role-assignments.go (1)
113-113: LGTM! More concise role assignment logging.The structured logging approach captures the essential identifiers (id and roleDefinitionId) without dumping the entire role assignment object, reducing log size while maintaining debugging utility.
cmd/list-app-role-assignments.go (1)
105-105: LGTM! Well-structured app role assignment logging.The logging now includes three key fields (id, appRoleId, and name) that provide sufficient context for troubleshooting while dramatically reducing log size compared to logging the entire object.
cmd/list-logic-apps.go (1)
114-114: LGTM! Cleaner logic app logging.The change reduces log verbosity by emitting only the id and name fields instead of the entire logic app object, maintaining essential debugging information while significantly reducing log size.
cmd/list-roles.go (1)
73-73: LGTM! Enhanced role logging with structured fields.The structured logging now includes three relevant fields (id, displayName, isEnabled) that provide comprehensive context for roles, making logs more useful for troubleshooting while avoiding the overhead of logging entire role objects.
cmd/list-subscription-owners.go (1)
91-91: LGTM! Improved subscription owner logging.The change provides structured logging with three key fields (principalId, role, and subscriptionId) that capture the essential information about subscription owners, reducing log size while maintaining full debugging capability.
cmd/list-key-vaults.go (1)
108-108: LGTM!The selective field logging (id, name, tenantId) provides sufficient identifying information while reducing log verbosity as intended.
cmd/list-storage-containers.go (1)
113-113: LGTM! Logging change achieves PR objective.The change successfully reduces verbose log output by logging only the key identifying fields (
id,name,tenant) instead of the entirestorageContainerobject. This maintains sufficient context for debugging while significantly reducing log size.cmd/list-management-group-role-assignments.go (1)
109-109: LGTM - Effective reduction of log verbosity!The change successfully replaces full-object logging with targeted field-level logging, significantly reducing log size while preserving essential debugging information (id, role definition, principal, and scope). All field paths exist on the struct and the code is safe—the logging is only reached when
item.Erroris nil.cmd/list-role-eligibility-schedule-instance.go (1)
22-24: These import additions appear to be marked incorrectly in the diff.The imports
os,os/signal, andtimeat lines 22-24 are marked as additions, but they're already present in the current code and actively used in lines 47, 52, and 56 (viasignal.NotifyContext(),os.Interrupt,os.Kill,time.Now(), andtime.Since()). Git history shows these imports are not new to this commit. Either these imports were reorganized rather than newly added, or the diff is incorrectly marking them as additions.cmd/list-role-assignments.go (1)
109-109: The logging statement on line 109 is safe and does not require a nil check. TheRoleDefinitionfield is a value type (UnifiedRoleDefinition, not a pointer), so when the API response does not include the roleDefinition field, it is zero-initialized with an empty string forDisplayName. Accessingitem.Ok.RoleDefinition.DisplayNamewill return an empty string rather than panic.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
cmd/list-subscription-user-access-admins.go (1)
91-91: Verbosity reduction achieved; consider adding more key fields for debugging.The change successfully reduces log verbosity by logging only the
Namefield instead of the entire object, which aligns with the PR objective.However, for debug logging at verbosity level 2, consider logging a few additional key fields (e.g.,
id,principalId) as mentioned in the PR description. This would provide better debugging context while keeping logs significantly smaller than full object dumps.🔎 Proposed enhancement to include additional key fields
- log.V(2).Info("found subscription user access admin", "name", subscriptionUserAccessAdmin.UserAccessAdmin.Name) + log.V(2).Info("found subscription user access admin", + "name", subscriptionUserAccessAdmin.UserAccessAdmin.Name, + "principalId", subscriptionUserAccessAdmin.UserAccessAdmin.Properties.PrincipalId, + "roleAssignmentId", subscriptionUserAccessAdmin.UserAccessAdmin.Id)Note: Please verify the exact field paths for
PrincipalIdandIdbased on themodels.SubscriptionUserAccessAdminstructure.cmd/list-key-vault-role-assignments.go (1)
109-109: Consider logging additional key fields for better debugging context.The PR description mentions logging "a few key fields (examples: id, name, principalId)" but this implementation logs only the Name field. Consider adding the Id field (for unique identification) and potentially PrincipalId and RoleDefinitionId from the Properties to better align with the stated objective while still maintaining compact logs.
🔎 Proposed enhancement with additional key fields
- log.V(2).Info("found key vault role assignment", "name", keyVaultRoleAssignment.RoleAssignment.Name) + log.V(2).Info("found key vault role assignment", + "id", keyVaultRoleAssignment.RoleAssignment.Id, + "name", keyVaultRoleAssignment.RoleAssignment.Name, + "principalId", keyVaultRoleAssignment.RoleAssignment.Properties.PrincipalId, + "roleDefinitionId", keyVaultRoleAssignment.RoleAssignment.Properties.RoleDefinitionId)cmd/list-service-principals.go (1)
73-73: Good change that aligns with PR objectives.The change from logging the entire service principal object to just the display name effectively reduces log verbosity as intended. The implementation is clean and straightforward.
Consider including the service principal ID alongside the name for better debugging and traceability:
🔎 Optional enhancement to include ID
- log.V(2).Info("found service principal", "name", item.Ok.DisplayName) + log.V(2).Info("found service principal", "name", item.Ok.DisplayName, "id", item.Ok.ID)This provides a unique identifier in logs without significantly increasing log size.
cmd/list-users.go (1)
87-87: LGTM! Logging change successfully reduces verbosity.The modification correctly implements the PR objective by logging only the user ID instead of the entire user object. This will significantly reduce log size while maintaining the ability to identify which user was processed.
Optionally, you could consider adding
displayNameoruserPrincipalNameto the log for improved human readability, though the ID alone is sufficient for debugging:💡 Optional enhancement for human readability
-log.V(2).Info("found user", "id", item.Ok.Id) +log.V(2).Info("found user", "id", item.Ok.Id, "displayName", item.Ok.DisplayName)cmd/list-managed-clusters.go (1)
109-109: LGTM! Logging change aligns with PR objectives.The change correctly reduces verbose log output by logging only the
namefield instead of the entiremanagedClusterobject. This directly addresses the PR's goal of reducing log file size while maintaining essential debugging information. The full data is still sent to the pipeline (lines 111-113), so no information is lost.Optional: Consider including the ID for enhanced traceability
If you'd like slightly more context for debugging without significantly impacting log size, you could also include the cluster ID:
-log.V(2).Info("found managed cluster", "name", managedCluster.Name) +log.V(2).Info("found managed cluster", "name", managedCluster.Name, "id", managedCluster.Id)However, given the PR's explicit goal of significantly reducing log size, the current approach with just the name is perfectly reasonable.
cmd/list-automation-account-role-assignments.go (1)
113-113: Logging refinement successfully reduces verbosity.The change effectively reduces log output size by logging only the
roleDefinitionIdfield instead of the entire object. This aligns well with the PR objective.Optionally, consider logging one or two additional fields (e.g.,
principalIdfrom the Assignee, or the assignmentid) to provide richer debugging context, as the PR description mentions logging "a few key fields." However, the current implementation is functional and achieves the primary goal.💡 Optional enhancement: Log additional key fields
If the Assignee structure supports it, consider logging a couple more fields for better debugging context:
-log.V(2).Info("found automation account role assignment", "roleDefinitionId", automationAccountRoleAssignment.RoleDefinitionId) +log.V(2).Info("found automation account role assignment", "roleDefinitionId", automationAccountRoleAssignment.RoleDefinitionId, "principalId", automationAccountRoleAssignment.Assignee.Properties.PrincipalId, "assignmentId", automationAccountRoleAssignment.Assignee.Id)Note: Verify field names match the actual structure of the Assignee object.
cmd/list-subscription-owners.go (1)
91-91: LGTM! Logging change successfully reduces verbosity.The change from logging the entire
subscriptionOwnerobject to just theOwner.Namefield effectively reduces log size while maintaining visibility into which owners are being processed, which aligns with the PR objective.Optional enhancement: Consider adding subscription context.
For improved traceability during debugging, you might consider logging the
subscriptionIdalongside the name (e.g.,"name", subscriptionOwner.Owner.Name, "subscriptionId", subscriptionOwner.SubscriptionId). This would still keep logs concise while providing clearer context about which subscription each owner belongs to, especially useful when processing multiple subscriptions concurrently.🔎 Optional: Add subscription context to the log
-log.V(2).Info("found subscription owner", "name", subscriptionOwner.Owner.Name) +log.V(2).Info("found subscription owner", "name", subscriptionOwner.Owner.Name, "subscriptionId", subscriptionOwner.SubscriptionId)cmd/list-management-group-descendants.go (1)
99-99: Consider including the ID field for better traceability.Logging only the name reduces verbosity as intended, but including the
Idfield would significantly improve debugging capability while still keeping logs much smaller than logging the full object.🔎 Proposed enhancement
- log.V(2).Info("found management group descendant", "name", item.Ok.Name) + log.V(2).Info("found management group descendant", "id", item.Ok.Id, "name", item.Ok.Name)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
.gitignorecmd/list-app-role-assignments.gocmd/list-automation-account-role-assignments.gocmd/list-automation-accounts.gocmd/list-container-registries.gocmd/list-container-registry-role-assignments.gocmd/list-function-app-role-assignments.gocmd/list-function-apps.gocmd/list-groups.gocmd/list-key-vault-role-assignments.gocmd/list-key-vaults.gocmd/list-logic-app-role-assignments.gocmd/list-logic-apps.gocmd/list-managed-cluster-role-assignments.gocmd/list-managed-clusters.gocmd/list-management-group-descendants.gocmd/list-management-group-role-assignments.gocmd/list-management-groups.gocmd/list-resource-group-role-assignments.gocmd/list-resource-groups.gocmd/list-role-assignment-policies.gocmd/list-role-assignments.gocmd/list-role-eligibility-schedule-instance.gocmd/list-roles.gocmd/list-service-principals.gocmd/list-storage-account-role-assignments.gocmd/list-storage-accounts.gocmd/list-storage-containers.gocmd/list-subscription-owners.gocmd/list-subscription-role-assignments.gocmd/list-subscription-user-access-admins.gocmd/list-subscriptions.gocmd/list-tenants.gocmd/list-users.gocmd/list-virtual-machine-role-assignments.gocmd/list-virtual-machines.gocmd/list-vm-scale-set-role-assignments.gocmd/list-vm-scale-sets.gocmd/list-web-apps.go
🚧 Files skipped from review as they are similar to previous changes (20)
- cmd/list-management-groups.go
- cmd/list-automation-accounts.go
- cmd/list-container-registry-role-assignments.go
- cmd/list-role-assignments.go
- cmd/list-managed-cluster-role-assignments.go
- cmd/list-logic-apps.go
- .gitignore
- cmd/list-vm-scale-sets.go
- cmd/list-management-group-role-assignments.go
- cmd/list-key-vaults.go
- cmd/list-logic-app-role-assignments.go
- cmd/list-app-role-assignments.go
- cmd/list-vm-scale-set-role-assignments.go
- cmd/list-subscriptions.go
- cmd/list-web-apps.go
- cmd/list-resource-group-role-assignments.go
- cmd/list-virtual-machines.go
- cmd/list-function-apps.go
- cmd/list-role-eligibility-schedule-instance.go
- cmd/list-storage-account-role-assignments.go
🧰 Additional context used
🧬 Code graph analysis (7)
cmd/list-virtual-machine-role-assignments.go (1)
models/azure/role_assignment.go (1)
RoleAssignment(31-43)
cmd/list-key-vault-role-assignments.go (1)
models/azure/role_assignment.go (1)
RoleAssignment(31-43)
cmd/list-groups.go (1)
constants/misc.go (1)
DisplayName(29-29)
cmd/list-subscription-role-assignments.go (1)
models/azure/role_assignment.go (1)
RoleAssignment(31-43)
cmd/list-role-assignment-policies.go (1)
models/azure/unified_role_management_policy_assignment.go (1)
UnifiedRoleManagementPolicyAssignment(22-31)
cmd/list-roles.go (1)
constants/misc.go (1)
DisplayName(29-29)
cmd/list-tenants.go (1)
client/query/params.go (1)
TenantId(41-41)
🔇 Additional comments (11)
cmd/list-tenants.go (1)
84-84: LGTM! Logging simplification achieves the PR objective.The change successfully reduces verbose log output by logging only the tenant ID instead of the full tenant object, which aligns perfectly with the stated goal of reducing log file sizes.
cmd/list-container-registries.go (1)
110-110: LGTM - Logging change successfully reduces verbosity.The change from logging the entire
containerRegistryobject to just thenamefield correctly implements the PR objective. This reduces log size significantly while maintaining sufficient observability for debugging purposes.cmd/list-storage-containers.go (1)
113-113: LGTM! Logging change aligns with PR objectives.The refactored log statement correctly reduces verbosity by logging only the storage container name instead of the entire object. This significantly decreases log size while retaining sufficient context for debugging success cases, especially since error logs (line 103) and completion logs (line 122) already provide additional context when needed.
cmd/list-function-app-role-assignments.go (1)
113-113: LGTM! Logging refactor aligns with PR objective.The change successfully reduces log payload by emitting only the
roleDefinitionIdinstead of the entire object, and also fixes a typo ("asignment" → "assignment"). This provides sufficient debugging information while significantly reducing log volume.cmd/list-roles.go (1)
73-73: LGTM! Concise logging improves observability.The refactored log statement emits only the
displayNamefield instead of the entire role object, reducing log volume while maintaining useful debugging context. The field access is safe as it follows error checking on Line 69.cmd/list-subscription-role-assignments.go (1)
109-109: LGTM! Targeted logging reduces payload effectively.The change replaces full object logging with just the
namefield from the role assignment, consistent with the PR's goal to reduce verbose log output. The field access is safe as it's protected by error handling on Line 102.cmd/list-role-assignment-policies.go (1)
76-76: LGTM! Nested field logging maintains clarity.The refactored log emits only the
policyIdfrom the nested policy object rather than the entire assignment structure. This is particularly effective for reducing log size given that role assignment policies contain complex rule structures. The access path is safe after error validation on Line 69.cmd/list-virtual-machine-role-assignments.go (1)
109-109: LGTM! Consistent pattern across role assignment commands.The change mirrors the refactoring in
list-subscription-role-assignments.go, logging only thenamefield instead of the entire role assignment object. This consistency across similar commands improves maintainability while achieving the PR's goal of reducing verbose log output.cmd/list-resource-groups.go (1)
105-105: LGTM! Logging change reduces verbosity effectively.The change from logging the entire
resourceGroupobject to justresourceGroup.Namesuccessfully achieves the PR objective of reducing log file size while maintaining useful diagnostic information.cmd/list-storage-accounts.go (1)
105-105: LGTM! Consistent logging improvement.The change from logging the entire
storageAccountobject to juststorageAccount.Namealigns with the PR's goal to reduce verbose log output. This is consistent with similar changes across other list commands.cmd/list-groups.go (1)
73-73: LGTM! Appropriate field selection for group logging.The change from logging the entire
itemto justitem.Ok.DisplayNameeffectively reduces log verbosity. UsingDisplayNameis the appropriate field for Azure AD groups and aligns with the PR objective.
Currently, when verbose logs are enabled, the output is extremely bloated, caused by log statements printing entire objects when they are found. In this MR, each action performed by the app has has its verbose log statement modified to only include a couple of key details (
id,name) rather than the entire object. Since the application will generate an output file containing all the info that would be in the trace logs, there is no need for duplicating all the details.Note
Demo:
Trace log file size comparison after changes:
-> Log file output reduced by ~6x
To test: run AzureHound with Verbose logs enabled and compare the new log output to the previous log output - the new one should be significantly smaller in size.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.