-
Notifications
You must be signed in to change notification settings - Fork 27
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
Look for metadata logs and display path #1007
base: main
Are you sure you want to change the base?
Look for metadata logs and display path #1007
Conversation
Signed-off-by: Mikayla Thompson <thomika@amazon.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1007 +/- ##
============================================
- Coverage 89.63% 80.21% -9.42%
- Complexity 0 2721 +2721
============================================
Files 43 365 +322
Lines 2364 13614 +11250
Branches 0 939 +939
============================================
+ Hits 2119 10921 +8802
- Misses 245 2122 +1877
- Partials 0 571 +571
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Mikayla Thompson <thomika@amazon.com>
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.
Thanks! This is a great idea.
I'd recommend some changes though.
- always print the filename of the log file at the beginning of the process (not all errors may be exceptions that kill the program).
- Use the pid in the filename - I think that ${java:pid} should work (see stackoverflow). That would make which logfile is relevant unambiguous (all you would need to know is the pid).
- All of the console invocations of other programs should do this (e.g. CreateSnapshot)
- We should be certain of the filepath that we're showing.
- If the python wrapper code needs to know where the logfiles are, it should make sure that it's looking in the right spot by specifying it and passing it to the program as it is launched. the log4j2 properties file would take an optional environment variable of the console program's logbase directory - not the pid part though. You'd still want to cons the pid variable in the properties file.
- In lieu of passing the directory, you could also have the console program print out its log location, though that doesn't make sense for every invocation (outside the console) and when it changes, this migration-console cli experience will break. Below is the code to use log4j2 directly (not slf) to get the filenames for the appenders.
LoggerContext context = (LoggerContext) LogManager.getContext(false);
Configuration config = context.getConfiguration();
// Loop through all appenders
config.getAppenders().forEach((name, appender) -> {
if (appender instanceof FileAppender) {
// Print the log file location
FileAppender fileAppender = (FileAppender) appender;
System.out.println("Log file location: " + fileAppender.getFileName());
}
});
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.
Thanks for making this change, this is a nice touch to help with debugging intuitively in the migration console.
This seems like an case where the program writing the logging output is better suited to know where the log file is located rather than in the console link application.
return self._run_command_and_build_result(command_runner) | ||
|
||
|
||
def get_latest_metadata_logs_path() -> Optional[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.
This method creates it creates coupling between this code and the logger code without an interface, if the behavior changed this code can broken with out an obvious reason. As Greg mentions there are ways we can determine from the logging system where the log file path is at that would prevent this cross-language coupling that are better served by being in the java side ecosystem.
What do you think about shifting this solution in that space?
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.
I disagree with the comment about the program knowing where logs are being written, though I do agree with the sentiment that the current situation should be unambiguous. Right now, none of the application code for any of the libraries or applications needs a hard dependency on Log4J2. Nearly every class, main ones included have dependencies on Slf4j. Log4J2 is bound into Slf4j at deployment/runtime. Having application code reach back into the logging system to find out where its logs live requires a much bigger interface. Furthermore, what if a deployment of an application is using a network logger (like what we previously had done w/ OTEL). That wouldn't be a filename at all. What would the expectations be for that code? How would one control whether that code would or wouldn't be activated to show the user something.
I see the log locations and the logging system that we're using entirely as a dependency that's injected. That's why I was recommending configuring the file outside the process in the process's configuration. If we want logs to be harmonized for apps that run on the console, we'll probably need that to be managed by the thing invoking the application. In that case, if the caller knew anyway, why require the application to also know?
Description
We saw in customer testing yesterday that not having the metadata logs path easily available complicates user debugging.
This PR checks for a new metadata migration log from the python code and prints it out in the output.
Issues Resolved
n/a
Testing
Manual (aghgh, I should add a unit test)
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.