Skip to content

Conversation

@tomkaragounis
Copy link
Contributor

@tomkaragounis tomkaragounis commented Feb 8, 2023

Should attempt to write debug info to log on windows if no debugger is available to recieve output

Debug logging will now be off by default

lib/pal/PAL.cpp Outdated
}
else
{
// Log to debug log file if enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Biggest complaint was usually if something goes wrong (e.g. customer blocks the end-point with firewall), these logs may grow substantially big on disk. Win 10 Storage Sense somewhat alleviates it by cleaning up the old session logs, but still it may get bad, especially on server flavor. This needs to be carefully tested and the logs should def be OFF by default. Like, no logs should ever be created by default unless the app config for debug/dev/test environments explicitly enables it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have shifted logging to be default off. Is there somewhere I can note the change of behavior for users to be aware of that they will need to add a setting to continue getting logs?
Could you be more specific on the type of testing you would like to see?

Copy link
Contributor

@maxgolov maxgolov Feb 8, 2023

Choose a reason for hiding this comment

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

Please check with @lalitb during weekly meeting. As long as you can avoid SDK spewing too many warnings into log file (ensuring that normally the file isn't being written into), it's all good.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to add, there is plan to add the support for log rotation/limiting in near future.

@tomkaragounis tomkaragounis force-pushed the thkarago/outputWindowsDebugLog branch from f5af205 to 231ed84 Compare February 9, 2023 20:39
Copy link
Contributor

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

Looks good to me. We can merge this after discussing in next week's community meeting.

@tomkaragounis tomkaragounis merged commit 63cc083 into main Feb 10, 2023
@tomkaragounis tomkaragounis deleted the thkarago/outputWindowsDebugLog branch February 10, 2023 20:49
tomkaragounis added a commit that referenced this pull request Feb 10, 2023
* Windows write to log if no debugger attached

* Added a preprocessor directive to turn on disk logging on windows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants