[C API] Add logging support (logger handle) by yuejiaointel · Pull Request #408 · intel/ScalableVectorSearch

yuejiaointel · 2026-10-01T21:07:41Z

Completes the C API logger-handle design from dev/c_api_logger_handle (initial API definition by Rafik Saliev, kept as the first commit). The C API user creates an svs_logger_h, choosing its output at creation (svs_logger_create(kind, path): none/stdout/stderr/file, or svs_logger_create_custom(user_logger): a C callback log(self, level, message)), sets level and pattern, and uses it as SVS's global logger (svs_set_default_logger). Indexes capture the default logger current when they are built or loaded and keep using it.

A per-index logger (svs_index_builder_set_logger) is not part of this PR; it comes in a follow-up after #407 (optional logger on svs::Vamana/svs::DynamicVamana build/assemble) and a nightly that includes it. The draft's declaration without a body was removed here so callers cannot link against it.

Changes on top of the initial definition

Bug fixes, no design change:

Fixed compile errors (spdlog logger name; sinks as shared_ptr).
Custom loggers use spdlog callback_sink_mt forwarding the bare message (the draft's shared static formatter was not thread-safe).
Custom logger ops are now validated (version/struct_size), the same way as the custom allocator.
Added svs_logger_free; implemented get_level, set_pattern and get_pattern.
Header doc fixes: lifetime and thread-safety rules for the callback; svs_set_default_logger documents that already-built indexes keep their logger.
Removed SVS_LOGGING_KIND_CUSTOM: custom loggers are created with svs_logger_create_custom, so the kind had no use (out-of-range kinds return SVS_ERROR_INVALID_ARGUMENT).

Design choices that differ from the initial definition (for discussion)

Where a choice was needed, I followed how VecSim/Redis already integrates SVS logging (VectorSimilarity/src/VecSim/algorithms/svs/svs.h).

#	Initial definition	Now	Why	Trade-off
D1	`svs_set_default_logger(NULL)` → error	NULL restores SVS default (`reset_to_default()`)	lets callers stop SVS using their callback before freeing it; needed by tests	NULL no longer an error
D2	`svs_logger_create()` read `SVS_LOG_SINK`/`SVS_LOG_LEVEL` on every call (with `file:X` each create truncated X)	output chosen at creation: `svs_logger_create(kind, path)` / `svs_logger_create_custom(user_logger)` (per review); no env read	no log-file wipe; one call gives a working logger	to change output, create another logger
D3	`set_kind`/`set_custom` could change a logger's output after indexes used it (inconsistently)	removed (per review): output is fixed at creation; level/pattern remain changeable	simpler, no surprise for existing indexes	—
D4	callback gets `%+` (`[time] [svs_c] [level] msg`)	default pattern `%v` (bare message) for all kinds	host loggers (Valkey, Redis) add their own time/level; like VecSim	stdout/file also bare by default; `set_pattern("%+")` restores
—	invalid level → Info	kept	—	alternative: return `SVS_ERROR_INVALID_ARGUMENT` like invalid kind

Code locations: D1 svs_c.cpp svs_set_default_logger; D2 svs_c.cpp svs_logger_create / svs_logger_create_custom, logger.hpp make_sink / make_custom_sink; D4 logger.hpp default_log_pattern. Custom loggers use spdlog callback_sink_mt and receive the bare message (pattern applies to stdout/stderr/file only). Logger types/helpers live in logger.hpp.

Known gap: changing or resetting the default logger does not affect indexes already built or loaded, so a custom callback and its self must stay valid while such indexes exist (documented and tested).

Testing (local)

Public-only build + unit tests (.github/scripts/build-c-api-bindings.sh + test-c-api-unit.sh): 14/14 pass. The new c_api_logging.cpp has 383 assertions.
LVQ/LeanVec + LTO build in the CI docker image (svs-manylinux228): build OK, 14/14 pass.
Packaged consumer test (test-c-api-bindings.sh): pass. The C99 consumer compiles SVS_INIT_LOGGING_OPS.
TSan (logging tests, native threadpool): clean. ASan+UBSan (logging tests): clean.
Plain-C99 end-to-end program against the installed package: custom callback receives build messages, svs_set_default_logger(NULL) stops new indexes from using it, file logger writes the log file.
Pre-existing, not from this PR: full-suite ASan flags c_api_index.cpp:1241 (allocator kind 999, UB) and leaks in c_api_dynamic_index.cpp:44-49. -std=c99 -Wpedantic -Werror fails on __VA_OPT__ in svs_c.h (filtered-search macro).

🤖 Generated with Claude Code

Builds on the initial logger handle API definition: - fix compile errors; add CallbackSink (base_sink + own formatter) so svs_logger_set_pattern applies to custom loggers and is thread-safe - validate custom logger ops version/struct_size - add svs_logger_free; implement get_level, set_pattern, get_pattern - svs_set_default_logger(NULL) restores the SVS default logger - new logger starts with no output (no env read / file reopen) - output changes apply in place to all users of a logger (dist_sink) - default pattern %v: callback receives the bare message - tests: c_api_logging.cpp; C99 consumer uses SVS_INIT_LOGGING_OPS

rfsaliev · 2026-10-02T14:29:27Z

+/// (svs_logger_set_pattern) applies. The trailing end-of-line added by the formatter is
+/// stripped before calling the user callback.
+/// The base_sink mutex serializes calls to the user callback.
+class CallbackSink final : public spdlog::sinks::base_sink<std::mutex> {


Why the new sink class defined rather than existing callback_sink from spdlog/sink/callback_sink.h?

Good point — switched to callback_sink_mt. A custom callback already gets level and its own self context, so it can format the line itself; it now receives the bare message, same as VecSim. svs_logger_set_pattern still applies to stdout/stderr/file outputs (documented in svs_c.h).

Custom loggers now use spdlog::sinks::callback_sink_mt with a lambda that forwards the bare message payload, level and self to the user callback, replacing the CallbackSink class. The logger pattern applies to stdout, stderr and file outputs only; custom callbacks format messages themselves.

rfsaliev · 2026-10-06T09:10:38Z

+/// (svs_logger_set_kind, svs_logger_set_custom, svs_logger_set_level,
+/// svs_logger_set_pattern, svs_logger_free) are not thread-safe. Do not call them
+/// concurrently with each other or with other functions using the same handle.
+SVS_API svs_logger_h svs_logger_create(svs_error_h out_err /*=NULL*/);


IMHO, an empty logger makes no sense for user experience.
I would propose to define 2 other creators instead:

/// @brief create a logger SVS_API svs_logger_h svs_logger_create(svs_logging_kind_t kind, const char* path, svs_error_h out_err); /// @brief create a custom logger SVS_API svs_logger_h svs_logger_create_custom(svs_logging_i user_logger, svs_error_h out_err);

And remove svs_logger_set_kind() and svs_logger_set_custom()
In such case, svs_logger::set_output() is not needed anymore - if user want to configure another output - it can be done by constructing another svs_logger_h;

Agreed — replaced with svs_logger_create(kind, path) and svs_logger_create_custom(user_logger); removed svs_logger_set_kind, svs_logger_set_custom and set_output().

rfsaliev

Please move logger definitions out-of svs_c.cpp

rfsaliev · 2026-10-06T12:59:16Z

+struct svs_logger {
+    // The handle keeps this one spdlog logger for its whole life. Indexes and the SVS
+    // global default logger hold references to it, so output, level and pattern changes
+    // made through the handle apply to them immediately.
+    svs::logging::logger_ptr impl;
+    // Single sink of `impl`; forwards to the current output sink. Replacing the output
+    // swaps the sink held here (dist_sink::set_sinks locks the sink mutex).
+    std::shared_ptr<spdlog::sinks::dist_sink_mt> output;
+    // Current pattern; returned by svs_logger_get_pattern().
+    std::string pattern = svs::c_runtime::default_log_pattern;
+
+    svs_logger()
+        : output{std::make_shared<spdlog::sinks::dist_sink_mt>()} {
+        // A new logger has no output until svs_logger_set_kind/set_custom is called.
+        output->set_sinks({svs::logging::null_sink()});
+        impl = std::make_shared<spdlog::logger>(svs::c_runtime::logger_name, output);
+        impl->set_level(spdlog::level::warn);
+        impl->set_pattern(pattern);
+    }
+
+    // Replace the output sink in place, keeping the current level and pattern.
+    void set_output(svs::logging::sink_ptr sink) {
+        // dist_sink only passes its formatter to sub-sinks present at set_pattern time,
+        // so give the new sink the current pattern before installing it.
+        sink->set_pattern(pattern);
+        output->set_sinks({std::move(sink)});
+    }
+};


To make svs_c.cpp cleaner, I would move-out this class definition to logger.hpp.
You may just keep the declaration here like:

Suggested change

struct svs_logger {

// The handle keeps this one spdlog logger for its whole life. Indexes and the SVS

// global default logger hold references to it, so output, level and pattern changes

// made through the handle apply to them immediately.

svs::logging::logger_ptr impl;

// Single sink of `impl`; forwards to the current output sink. Replacing the output

// swaps the sink held here (dist_sink::set_sinks locks the sink mutex).

std::shared_ptr<spdlog::sinks::dist_sink_mt> output;

// Current pattern; returned by svs_logger_get_pattern().

std::string pattern = svs::c_runtime::default_log_pattern;

svs_logger()

: output{std::make_shared<spdlog::sinks::dist_sink_mt>()} {

// A new logger has no output until svs_logger_set_kind/set_custom is called.

output->set_sinks({svs::logging::null_sink()});

impl = std::make_shared<spdlog::logger>(svs::c_runtime::logger_name, output);

impl->set_level(spdlog::level::warn);

impl->set_pattern(pattern);

}

// Replace the output sink in place, keeping the current level and pattern.

void set_output(svs::logging::sink_ptr sink) {

// dist_sink only passes its formatter to sub-sinks present at set_pattern time,

// so give the new sink the current pattern before installing it.

sink->set_pattern(pattern);

output->set_sinks({std::move(sink)});

}

};

// Defined in logger.hpp

struct svs_logger;

Done — moved to logger.hpp.

rfsaliev · 2026-10-06T13:00:24Z

+inline svs::logging::Level to_logging_level(svs_log_level_t level) {
+    switch (level) {
+        case SVS_LOG_LEVEL_TRACE:
+            return svs::logging::Level::Trace;
+        case SVS_LOG_LEVEL_DEBUG:
+            return svs::logging::Level::Debug;
+        case SVS_LOG_LEVEL_INFO:
+            return svs::logging::Level::Info;
+        case SVS_LOG_LEVEL_WARN:
+            return svs::logging::Level::Warn;
+        case SVS_LOG_LEVEL_ERROR:
+            return svs::logging::Level::Error;
+        case SVS_LOG_LEVEL_CRITICAL:
+            return svs::logging::Level::Critical;
+        case SVS_LOG_LEVEL_OFF:
+            return svs::logging::Level::Off;
+        default:
+            return svs::logging::Level::Info;
+    }
+}


I would move this function to logger.hpp as well...

Done — moved to_logging_level to logger.hpp.

A logger handle now gets its output at creation and keeps it for life: svs_logger_create(kind, path) for the built-in outputs (none, stdout, stderr, file append/truncate) and svs_logger_create_custom(user_logger) for a user callback. svs_logger_set_kind, svs_logger_set_custom and the dist_sink-based in-place output swap are removed; to log somewhere else, create another logger. Level and pattern setters are unchanged and still apply to everything already using the logger. struct svs_logger, to_logging_level() and the sink construction move from svs_c.cpp to logger.hpp. Docs, the C consumer program and the logging tests follow the new API.

…ix docs - Remove the svs_index_builder_set_logger declaration: it has no definition in this change, so any caller would fail to link. Per-index loggers come in a follow-up once the core orchestrators accept a logger. - Remove SVS_LOGGING_KIND_CUSTOM: custom loggers are created with svs_logger_create_custom, so the kind had no use and svs_logger_create only rejected it. The remaining values keep their numbering; out-of-range kinds still return SVS_ERROR_INVALID_ARGUMENT. - svs_set_default_logger docs: indexes capture the default logger current when they are built or loaded and keep using it; a later default change does not affect them, so a custom callback and its self must outlive such indexes. NULL rebuilds the built-in default from SVS_LOG_SINK/SVS_LOG_LEVEL. - svs_c.cpp: drop the redundant forward declaration of svs_logger and the duplicate <svs/core/logging.h> include (both come from logger.hpp). - Tests: check the exact bare message "Number of syncs: 40" instead of scanning for a '[' prefix; add "Default Logger Change Does Not Affect Built Index" (dynamic index keeps logging to the captured logger after svs_set_default_logger(NULL)); kind value 5 is now rejected.

rfsaliev

LGFM, just few questions according to API definitions and corner cases.

rfsaliev · 2026-10-07T10:36:56Z

+/// Maps a C API log level to the SVS logging level. Unknown values map to Info.
+inline svs::logging::Level to_logging_level(svs_log_level_t level) {
+    switch (level) {
+        case SVS_LOG_LEVEL_TRACE:
+            return svs::logging::Level::Trace;
+        case SVS_LOG_LEVEL_DEBUG:
+            return svs::logging::Level::Debug;
+        case SVS_LOG_LEVEL_INFO:
+            return svs::logging::Level::Info;
+        case SVS_LOG_LEVEL_WARN:
+            return svs::logging::Level::Warn;
+        case SVS_LOG_LEVEL_ERROR:
+            return svs::logging::Level::Error;
+        case SVS_LOG_LEVEL_CRITICAL:
+            return svs::logging::Level::Critical;
+        case SVS_LOG_LEVEL_OFF:
+            return svs::logging::Level::Off;
+        default:
+            return svs::logging::Level::Info;
+    }
+}


IMHO, the "Unknown values" case should be documented for svs_logger_set_level() in svs_c.h.
But I would recommend to throw a std::invalid_argument for the case - it will allow user to early identify issues in the code which calls to SVS API.
For now, user gets "silent" behavior for wrong calls.

Done — invalid levels now return SVS_ERROR_INVALID_ARGUMENT; documented on svs_logger_set_level.

rfsaliev · 2026-10-07T10:56:47Z

+            INVALID_ARGUMENT_IF(logger == nullptr, "Logger must not be null");
+            EXPECT_ARG_NOT_NULL(pattern);
+            auto new_pattern = std::string(pattern);
+            logger->impl->set_pattern(new_pattern);


What will happen if new_pattern.empty() (or *pattern == '\0')?
If such case is unacceptable, then I would guard this with:

Suggested change

logger->impl->set_pattern(new_pattern);

INVALID_ARGUMENT_IF(new_pattern.empty(), "Pattern should not be empty");`

logger->impl->set_pattern(new_pattern);

Good catch — an empty pattern would drop the message text; added your guard.

rfsaliev · 2026-10-07T10:58:58Z

+
+/// @brief Set format pattern for a logger
+/// @param logger The logger handle
+/// @param pattern The format pattern to set, in spdlog syntax (e.g. "[index A] %v")


Does it make sense to add a link to the spdlog documentation?

Yes — added a link to spdlog's pattern syntax.

…ble (#407) Adds an optional per-index logger to the orchestrator front doors, so bindings (first: the C API, #408) can give each index its own logger. VecSim already does this by calling the lower-level builders directly. ## Change A trailing `svs::logging::logger_ptr logger = svs::logging::get()` parameter is added to: - `svs::Vamana::build` - `svs::Vamana::assemble` (path-based overload) - `svs::DynamicVamana::build` - `svs::DynamicVamana::assemble` (path-based overload; the logger goes after `debug_load_from_static`) Each one forwards it to `auto_build` / `auto_assemble` / `auto_dynamic_build` / `auto_dynamic_assemble`, which already accept a logger. The default is the same value those functions used before, so existing callers are unchanged. Not changed: the stream-based `assemble` overloads, because they end in a `DataLoaderArgs&&...` pack. This is a minimal subset of #128. ## Testing - New tests: "Vamana Per-Index Logger" and "DynamicVamana Per-Index Logger" (`tests/svs/orchestrators/`). They cover a custom logger vs. the global default, for both build and assemble. - `[managers][vamana],[managers][dynamic_vamana]`: all pass (353 assertions, 6 cases). `[logging]`: all pass (602 assertions, 10 cases). Build with no warnings. - Python bindings were not rebuilt locally; CI will cover them. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

[C API] Initial logging API definition with Logger handle.

d8c502f

yuejiaointel mentioned this pull request Oct 1, 2026

Add optional per-index logger to Vamana/DynamicVamana build and assemble #407

Merged

yuejiaointel force-pushed the yuejiao/c-api-logging branch from 691b59a to 9591762 Compare October 1, 2026 21:09

rfsaliev reviewed Oct 2, 2026

View reviewed changes

rfsaliev reviewed Oct 6, 2026

View reviewed changes

yuejiaointel added 2 commits October 6, 2026 10:56

yuejiaointel marked this pull request as ready for review October 6, 2026 22:36

yuejiaointel requested review from ahuber21 and ethanglaser as code owners October 6, 2026 22:36

yuejiaointel requested a review from rfsaliev October 6, 2026 22:36

[C API] Trim logger comments to header style

3ee9e29

rfsaliev reviewed Oct 7, 2026

View reviewed changes

[C API] Reject invalid log level and empty pattern; link pattern syntax

eef772c

	logger->impl->set_pattern(new_pattern);
	INVALID_ARGUMENT_IF(new_pattern.empty(), "Pattern should not be empty");`
	logger->impl->set_pattern(new_pattern);

Conversation

yuejiaointel commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes on top of the initial definition

Design choices that differ from the initial definition (for discussion)

Testing (local)

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!

rfsaliev left a comment

Choose a reason for hiding this comment

Uh oh!

rfsaliev Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

rfsaliev left a comment

Choose a reason for hiding this comment

Uh oh!

rfsaliev Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rfsaliev Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yuejiaointel commented Oct 1, 2026 •

edited

Loading

rfsaliev Oct 6, 2026 •

edited

Loading

rfsaliev Oct 7, 2026 •

edited

Loading

rfsaliev Oct 7, 2026 •

edited

Loading