[AIMIGRAPHX-371] Add logger with basic sinks #4469

eddieliao · 2025-11-26T00:30:21Z

Motivation

Adds a logger to MIGraphX that can log to consoles and files with specific severity.

Windows specific logs and conversion of current usage of cout and cerr will come in a later PR.

Technical Details

Introduces spdlog as a dependency which is used as the back-end for the MIGraphX logger. Output to the logger can be through either stream or variadic function. Messages can have one of five severity levels: error, warn, info, debug, or trace.

Changelog Category

- Added: New functionality.
- Changed: Changes to existing functionality.
- Removed: Functionality or support that has been removed. (Compared to a previous release)
- Optimized: Component performance that has been optimized or improved.
- Resolved Issues: Known issues from a previous version that have been resolved.
- Not Applicable: This PR is not to be included in the changelog.

Copilot

Pull request overview

This PR adds a logging system to MIGraphX using spdlog as the backend. The implementation provides five severity levels (NONE, ERROR, WARN, INFO, DEBUG, TRACE) with both stream and variadic function interfaces. The logger supports console (stderr) and file outputs, with configurable severity levels via environment variable or CLI options.

Introduces spdlog v1.16.0 as a dependency
Implements a wrapper around spdlog for MIGraphX-specific logging needs
Adds CLI options (--log-level, --log-file) to the driver for runtime configuration
Includes comprehensive unit tests covering various logging scenarios

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/include/migraphx/logger.hpp	Header defining the logger API with severity enum, stream/function interfaces, and print templates
src/logger.cpp	Implementation of logger using spdlog backend with sink management and severity mapping
test/logger_test.cpp	Comprehensive test suite covering severity levels, logging interfaces, and edge cases
src/driver/main.cpp	Integration of logger CLI options for runtime configuration
src/CMakeLists.txt	Build system update to include logger.cpp
requirements.txt	Addition of spdlog v1.16.0 dependency

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-26T00:34:58Z

src/driver/main.cpp

+           ap.append(),
+           ap.nargs(2));


The ap.nargs(2) for --log-file appears incorrect. The help text suggests accepting multiple log files (--log-file file1.log file2.log ...), but nargs(2) will limit it to exactly 2 arguments. Consider using a different approach such as ap.nargs_at_least(1) if available, or removing the nargs constraint entirely since the parsing logic at lines 186-191 already handles consuming all values until the next flag.

Suggested change

ap.append(),

ap.nargs(2));

ap.append());

Copilot · 2025-11-26T00:34:59Z

src/logger.cpp

+    static spdlog::logger* migraphx_logger =
+        new spdlog::logger("migraphx_logger", begin(sinks), end(sinks));


The logger is allocated with new but never deleted, creating a memory leak. While this may be intentional for a singleton that lives for the program's lifetime, consider using a smart pointer (std::unique_ptr) or documenting this intentional leak to clarify the design decision.

Copilot · 2025-11-26T00:34:59Z

src/include/migraphx/logger.hpp

+    {
+        template <class T>
+        stream(severity ps, T&& x, source_location ploc = source_location::current())
+            : s(ps), loc(ploc), enabled(is_enabled(s))


[nitpick] The stream constructor takes severity as the first template parameter but then also accepts it as a runtime parameter. This design could be confusing. Consider adding a comment explaining why both are needed or simplifying the design to use only the template parameter.

codecov · 2025-11-26T02:07:23Z

Codecov Report

❌ Patch coverage is 86.36364% with 9 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/logger.cpp	79.55%	9 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4469      +/-   ##
===========================================
- Coverage    92.21%   92.20%   -0.01%     
===========================================
  Files          561      563       +2     
  Lines        27228    27294      +66     
===========================================
+ Hits         25108    25165      +57     
- Misses        2120     2129       +9

Files with missing lines	Coverage Δ
src/include/migraphx/logger.hpp	`100.00% <100.00%> (ø)`
src/logger.cpp	`79.55% <79.55%> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

pfultz2 · 2025-11-26T14:28:55Z

src/include/migraphx/logger.hpp

+
+namespace migraphx {
+namespace log {
+inline namespace MIGRAPHX_INLINE_NS {


The inline namespace should come after migraphx.

pfultz2 · 2025-11-26T15:12:19Z

src/logger.cpp

+{
+    static size_t level = value_of(MIGRAPHX_LOG_LEVEL{}, static_cast<size_t>(severity::INFO));
+    return level;
+}


This isnt thread-safe. We should do a access_log_level HOF:

template<class F> static void access_log_level(F f) { static std::mutex m; static size_t level = value_of(MIGRAPHX_LOG_LEVEL{}, static_cast<size_t>(severity::INFO)); std::lock_guard<std::mutex> lock(m); f(level); }

pfultz2 · 2025-11-26T15:23:35Z

src/include/migraphx/logger.hpp

+
+void set_log_level(severity s);
+
+void add_file_logger(std::string_view filename, severity s = severity::INFO);


We should define a sink as using sink = std::function<void(severity, std::string_view, source_location)> and then do add_sink(severity level, sink s). Each sink should have there own log level.

Also add_sink should return an id so the user can remove or modify the sink later on. The stderror sink should be id 0.

pfultz2 · 2025-11-26T15:25:57Z

src/include/migraphx/logger.hpp

+
+bool is_enabled(severity s);
+
+void set_log_level(severity s);


This should only set the severity for the stderror. Probably should be named set_stderr_severity.

Nevermind, we could change the level for all sinks by passing an id. So this could be set_severity(severity s, std::size_t id = 0).

pfultz2 · 2025-11-26T15:28:47Z

src/logger.cpp

+static void init_stderr_logger()
+{
+    static bool initialized = false;
+    if(!initialized)


This isnt thread-safe, using static-init with a function call or std::call_once should make it correct.

pfultz2 · 2025-11-26T15:31:00Z

src/logger.cpp

+        // Set info level to use default terminal color
+        stderr_sink->set_color(spdlog::level::info, "");
+        logger->sinks().push_back(stderr_sink);
+        logger->set_level(to_spdlog_level(static_cast<severity>(get_log_level())));


I dont think we need to use spdlog for stderror. In fact, I dont think we need it at all. Users can add sinks to spdlog when they need such logging capability.

pfultz2 · 2025-11-26T15:42:03Z

test/logger_test.cpp

+    }
+}
+
+int main(int argc, const char* argv[]) { test::run(argc, argv); }


Having unit tests is great. Nice work!

It would be even better to have multithreading tests as well.

Also, I would love to have TSAN running in our CI so we could more easily catch multithreading issues.

eddieliao added 13 commits September 20, 2025 00:08

Add basic logging functionality

84d885b

Formatting

5203de7

Refactor logger to use variadic function and add record function

625a627

Formatting

b4a6ff6

Add console and file logging using spdlog

213fe9c

Formatting

c818ecd

Add multi file logging; fix params not showing in logging

b6599ca

Fix color logging

1fdb940

Add windows color logger

d8a8414

Add file-specific severity option

0cd773c

Remove windows specific sinks, add unit tests

10a9ae1

Formatting

5c1fcff

Cleanup

60b64c8

eddieliao self-assigned this Nov 26, 2025

eddieliao linked an issue Nov 26, 2025 that may be closed by this pull request

MIGraphX Logging #4094

Open

Merge branch 'develop' into logger

666e2c1

eddieliao requested a review from Copilot November 26, 2025 00:32

Copilot started reviewing on behalf of eddieliao November 26, 2025 00:33 View session

Copilot finished reviewing on behalf of eddieliao November 26, 2025 00:34

Copilot AI reviewed Nov 26, 2025

View reviewed changes

pfultz2 reviewed Nov 26, 2025

View reviewed changes

		static spdlog::logger* migraphx_logger =
		new spdlog::logger("migraphx_logger", begin(sinks), end(sinks));


		void set_log_level(severity s);

		void add_file_logger(std::string_view filename, severity s = severity::INFO);

[AIMIGRAPHX-371] Add logger with basic sinks #4469

Are you sure you want to change the base?

[AIMIGRAPHX-371] Add logger with basic sinks #4469

Conversation

eddieliao commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Technical Details

Changelog Category

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eddieliao commented Nov 26, 2025 •

edited

Loading

codecov bot commented Nov 26, 2025 •

edited

Loading