-
Notifications
You must be signed in to change notification settings - Fork 260
chore: close PR - logging changes added verbosity without value #4127
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
base: master
Are you sure you want to change the base?
Conversation
… and CNS components Co-authored-by: prmathur-microsoft <20328314+prmathur-microsoft@users.noreply.github.com>
Co-authored-by: prmathur-microsoft <20328314+prmathur-microsoft@users.noreply.github.com>
…omponents Co-authored-by: prmathur-microsoft <20328314+prmathur-microsoft@users.noreply.github.com>
prmathur-microsoft
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments test
rbtr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
except for a couple of places where an NC or endpoint ID is added, this isn't enhancing anything.
rewording the logs to be more verbose, restating obvious, redundant, or recently logged things, replacing "NC" with "Network Container" or "IP" with "IP Address"... are all things that do not actually improve log quality or readability.
As stated in the description of the PR the purpose was to reword and increase readability. |
|
Yes, I read that. I think that is not the net effect of these changes, despite Copilot confidently saying so. A couple samples:
Also, "IP" -> "IP Address" - redundant, longer For logs, longer is not better. Writing out well understand acronyms is not better. Logs need to be concise and information dense. More to read is actually worse for readability. |
|
@copilot close this PR. |
I understand. Closing this PR as requested. The feedback from @rbtr about keeping logs concise and information-dense is valid - expanding acronyms like "NC" to "Network Container" or "IP" to "IP Address" added verbosity without improving clarity. Future improvements should focus on adding missing context (like IDs where they're absent) rather than rewording existing messages. |
Reason for Change:
This PR attempted to improve logging messages across network and CNS components by adding contextual identifiers. However, based on review feedback, the changes increased verbosity without adding meaningful value, making logs harder to read rather than more useful.
Issue Fixed:
N/A - PR being closed without merge
Changes:
This PR made the following changes that are not being merged:
Examples of problematic changes:
Feedback Received:
Logs need to be concise and information-dense. Longer messages are worse for readability. Future improvements should focus on adding missing context (like IDs where absent) rather than rewording or expanding well-understood acronyms.
Requirements:
Notes:
PR is being closed. Will work on smaller, more focused changes that add actual contextual information without unnecessary verbosity.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.