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

revert: explicitly write wide string characters to console #193

Merged
merged 2 commits into from
Sep 25, 2024

Conversation

iankingori
Copy link
Contributor

reverts changes from this PR.

Copy link
Contributor

@CharityKathure CharityKathure left a comment

Choose a reason for hiding this comment

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

LGTM

@Raffledoocious
Copy link

Raffledoocious commented Sep 18, 2024

Hey all! Thanks for the quick fix on this one. I was playing around with the codebase yesterday and I suspected it was the changes reverted in this PR. I pulled down this branch and deployed the exe to my container in ECS. I have logs again but now LogMonitor shows a different error when monitoring my directory which was previously working.

Would you prefer a separate bug for this?

Expected

[2024-09-18T15:05:37.000Z][LOGMONITOR] INFO: Log directory \\?\C:\app\logs\app.com does not exist. LogMonitor will wait for 1800 seconds for the directory to be created.

Immediately after

[2024-09-18T15:05:37.000Z][LOGMONITOR] ERROR: Failed to monitor changes in directory \\?\C:\app\logs\app.com Error: 3
[2024-09-18T15:05:37.000Z][LOGMONITOR] ERROR: Failed to open log directory handle. Directory: \\?\C:\app\logs\app.com Error=3 
[2024-09-18T15:05:37.000Z][LOGMONITOR] ERROR: Failed to start log file monitor. Log files in a directory '\\?\C:\app\logs\app.com' will not be monitored. Error: 3

Serilog configuration in appsettings.json

    "WriteTo": {
      "File": {
        "Name": "Logger",
        "Args": {
          "configureLogger": {
            "WriteTo": {
              "FileInnerSink": {
                "Name": "File",
                "Args": {
                  "formatter": "Serilog.Formatting.Json.JsonFormatter, Serilog",
                  "path": "C:\\app\\logs\\app.com\\Va.Admin.log",
                  "rollingInterval": "Day",
                  "rollOnFileSizeLimit": true
                }
              }
            },
            "Filter": {
              "RemoveAccessLogs": {
                "Name": "ByExcluding",
                "Args": {
                  "expression": "SourceContext = 'App.Web.Logging.AccessLogHttpModule'"
                }
              }
            }
          }
        }
      },
      "AccessLogs": {
        "Name": "Logger",
        "Args": {
          "configureLogger": {
            "WriteTo": {
              "AccessLogsInnerSink": {
                "Name": "File",
                "Args": {
                  "formatter": "Serilog.Formatting.Json.JsonFormatter, Serilog",
                  "path": "C:\\app\\logs\\app.com\\Access.log",
                  "rollingInterval": "Day",
                  "rollOnFileSizeLimit": true
                }
              }
            },
            "Filter": {
              "IncludeAccessLogs": {
                "Name": "ByIncludingOnly",
                "Args": {
                  "expression": "SourceContext = 'App.Commons.Web.Logging.AccessLogHttpModule'"
                }
              },
              "ExcludeHealthChecks": {
                "Name": "ByExcluding",
                "Args": {
                  "expression": "SourceContext = 'App.Commons.Web.Logging.AccessLogHttpModule' and EndsWith(RequestPath, '/alive.ashx') and @l = 'Information'"
                }
              }
            }
          }
        }
      }
    }

LogMonitorConfig.json

{
  "LogConfig": {
    "logFormat": "custom",
    "sources": [
      {
        "customLogFormat": "%Message%",
        "type": "File",
        "directory": "C:\\app\\logs\\app.com",
        "filter": "*.log",
        "includeSubdirectories": false,
        "includeFileNames": false,
        "waitInSeconds": 1800
      }
    ]
  }
}

@Raffledoocious
Copy link

Raffledoocious commented Sep 18, 2024

I did some investigating. Bear with me here because I am not a C++ developer. It looks like the changes to FileMonitorUtilities.cpp are what is causing the break - Comparison with 2.0.2.

I suspect it is the return of INVALID_HANDLE_VALUE which causes the file monitoring thread to exit and stop monitoring a directory. This is not ERROR_SUCCESS so the monitoring thread exits.

@Raffledoocious
Copy link

I did confirm the above theory. Using my local build of LogMonitor.exe I now have custom logs flowing again from my container in ECS! I love the custom logs feature by the way and it was my main impetus for upgrading. I now have structured logs flowing to my Log Group.

I'll let you all do with what you want from my comments. If you need anything else from me let me know. Thanks again for the quick response!

@iankingori
Copy link
Contributor Author

Thanks @Raffledoocious for testing this, working on the bug you've mentioned above

@TinaMor
Copy link
Contributor

TinaMor commented Sep 19, 2024

@Raffledoocious Could you confirm if the path C:\app\logs\ exists before running LogMonitor?

By design, the parent directory must exist. In this case, the path is C:\app\logs\app.com, and the parent directory is C:\app\logs\

// NOTE: For nested directories, the parent dir must exist. If it does not, this will raise an error

The error is thrown here:

L"Failed to monitor changes in directory %s. Error: %lu",

TODO: Update README.md

@Raffledoocious
Copy link

@TinaMor Sure thing. I could create it in my Dockerfile and ensure it does exist on container start. I see you have a TODO for the README which would help clarify the directory existing is now expected behavior.

Does this message also need adjustment if this behavior is changed?

[2024-09-18T15:05:37.000Z][LOGMONITOR] INFO: Log directory \\?\C:\app\logs\app.com does not exist. LogMonitor will wait for 1800 seconds for the directory to be created.

@TinaMor
Copy link
Contributor

TinaMor commented Sep 20, 2024

LogMonitor expects that the parent directory (for nested directories) to exist. For example: the directory path is “C:\app\logs\app.com”. It is expected that “C:\app\logs\” exists before LogMonitor starts.

We will update the README.md with this.
Maybe a bug/feature to wait for nested folders… 🤔?

@Raffledoocious
Copy link

Awesome, thanks for the clarification! I'm happy to test out anything for you all if you need it on this PR otherwise I'll look for the next release.

@iankingori iankingori merged commit d36c314 into main Sep 25, 2024
7 checks passed
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.

[BUG] Logs are no longer output when upgrading to 2.1.0 version of LogMonitor
5 participants