-
Couldn't load subscription status.
- Fork 69
Suppress unnecessary error log for ERROR_NOT_SUPPORTED cases #215
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: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR fixes a JSON parser bug where numeric values caused parsing failures when they weren't the last element in an object. The fix corrects buffer pointer advancement to avoid skipping structural characters after numbers.
- Removed incorrect buffer pointer advancement after parsing numbers to preserve terminating characters
- Added error handling improvements to suppress benign ERROR_NOT_SUPPORTED logging
- Updated code comments to clarify the numeric parsing behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| JsonFileParser.cpp | Fixed buffer pointer advancement logic and updated comments for numeric value parsing |
| LogFileMonitor.cpp | Added conditional check to suppress logging of benign ERROR_NOT_SUPPORTED errors |
| if (GetLastError() != ERROR_NOT_SUPPORTED) | ||
| { | ||
| logWriter.TraceError( | ||
| Utility::FormatString( | ||
| L"Error in log file monitor. Failed to open file %ws. Error = %d", | ||
| fileName.c_str(), | ||
| GetLastError() | ||
| ).c_str() | ||
| ); | ||
| } |
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.
@bobsira I think similar workaround is needed here
windows-container-tools/LogMonitor/src/LogMonitor/LogFileMonitor.cpp
Lines 1082 to 1088 in 09983f6
| logWriter.TraceError( | |
| Utility::FormatString( | |
| L"Error in log file monitor. Failed to query file ID. File: %ws. Error: %d", | |
| fullLongPath.c_str(), | |
| status | |
| ).c_str() | |
| ); |
I'm testing your LogMonitor.exe build you linked in #214 (comment) and
The waitInSeconds: 10 does not seem to help delay the files query in this particular case.
My LogMonitorConfig.json is this
{
"LogConfig": {
"sources": [
{
"includeSubdirectories": false,
"filter": "*.log",
"directory": "C:\\Logs",
"waitInSeconds": 10,
"type": "File",
"includeFileNames": true
}
]
}
}The fix for ordering of the waitInSeconds seem to work.
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.
@mloskot please confirm if the issue still exists in this binary -> https://minikubevhdimagebuider.blob.core.windows.net/versions/LogMonitor.exe
It has the change you've suggested. if it's there we can move this fix to a different PR and let this handle the parser fix
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.
@bobsira Hmm, I've just tested LogMonitor.exe from that URL
Invoke-WebRequest -Uri 'https://minikubevhdimagebuider.blob.core.windows.net/versions/LogMonitor.exe' -OutFile C:/LogMonitor.exe -UseBasicParsing
and this version is reported inside the container - am I using the right custom ad-hoc build of yours?
but I can't see any difference
Here is my config inside the container
and logs location
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.
Hmmm, strange that should be the binary with the change. I'll go ahead and split the PR for now. I have a feeling this issue might be coming from another place in the code as well so let's have it addressed in a different PR.
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.
Good idea. Let's take baby steps. The overall direction seems fine though 😊
This pull request improves error handling in the
LogFileMonitorcomponent by suppressing unnecessary error logs for benignERROR_NOT_SUPPORTEDcases. This helps prevent log pollution and ensures only actionable errors are logged.Error handling improvements:
LogFileMonitor::InitializeDirectoryChangeEventsQueue()to suppress logging forERROR_NOT_SUPPORTED, reducing unnecessary log entries for benign errors.LogFileMonitor::LogFileAddEventHandler()to only log errors when the status is notERROR_NOT_SUPPORTED, further preventing log pollution.Logging clarity: