Small library for metrics scraping in C++

Question

I did an internship test project (source: https://github.com/SynI20N/VKInfo). Looking for some middle or senior devs to rate it and point out potential problems with my code. Follow up: How would you test my code like people do it in production?

metrics_logger.h:

 #ifndef METRICS_LOGGER_H_
 #define METRICS_LOGGER_H_
 
 #include <string>
 #include <atomic>
 #include <thread>
 
 #include "metrics_registry.h"
 
 namespace VkInfo {
 
 /**
 * @brief Periodically logs metric values to a file.
 * 
 * MetricsLogger runs in a background thread, collecting metric values from a MetricsRegistry
 * at fixed intervals and writing them to a log file with timestamps. After logging, each metric
 * is reset. This logger is thread-safe and non-blocking for metric producers.
 */
 class VKINFO_API MetricsLogger {
 public:
 /**
 * @brief Constructs a MetricsLogger instance.
 * 
 * @param registry Reference to the MetricsRegistry containing all metrics to log.
 * @param filename Name of the file to write log entries to.
 * @param interval Logging interval (e.g., every second).
 */
 MetricsLogger(MetricsRegistry& registry, const std::string& filename, std::chrono::milliseconds interval);
 
 /**
 * @brief Destructor for MetricsLogger.
 * 
 * Ensures the background thread is stopped before destruction.
 */
 ~MetricsLogger();
 
 /**
 * @brief Starts the background logging thread.
 * 
 * Logs metric data to the file at the specified interval.
 */
 void start();
 
 /**
 * @brief Stops the background logging thread safely.
 */
 void stop();
 
 /**
 * @brief Retrieves all currently registered metrics from the registry.
 * 
 * @return A vector of shared pointers to IMetric instances.
 */
 std::vector<std::shared_ptr<IMetric>> get_all_metrics();
 
 private:
 MetricsRegistry& registry_; ///< Reference to the metric registry.
 std::string filename_; ///< Output log file name.
 std::chrono::milliseconds interval_; ///< Logging frequency.
 std::atomic<bool> running_; ///< Indicates if the logger is running.
 std::thread worker_; ///< Background thread for logging.
 
 /**
 * @brief Main logging loop executed in a background thread.
 * 
 * Gathers metrics, writes them to the file, and resets them.
 */
 void run();
 
 /**
 * @brief Gets the current system time as a formatted timestamp string.
 * 
 * @return Timestamp in the format "YYYY-MM-DD HH:MM:SS.mmm".
 */
 std::string get_current_timestamp() const;
 };
 
 } // namespace VkInfo
 
 #endif // METRICS_LOGGER_H_

metrics_logger.cc:

 #include <iostream>
 #include <fstream>
 #include <sstream>
 #include <chrono>
 #include <iomanip>
 
 #include "../include/metrics_logger.h"
 
 namespace VkInfo {
 
 MetricsLogger::MetricsLogger(MetricsRegistry& registry,
 const std::string& filename,
 std::chrono::milliseconds interval)
 : registry_(registry),
 filename_(filename),
 interval_(interval),
 running_(false) {}
 
 MetricsLogger::~MetricsLogger() {
 stop();
 }
 
 void MetricsLogger::start() {
 if (running_) return;
 running_ = true;
 worker_ = std::thread(&MetricsLogger::run, this);
 }
 
 void MetricsLogger::stop() {
 if (!running_) return;
 running_ = false;
 if (worker_.joinable()) {
 worker_.join();
 }
 }
 
 std::vector<std::shared_ptr<IMetric>> MetricsLogger::get_all_metrics() {
 return registry_.get_all_metrics();
 }
 
 std::string MetricsLogger::get_current_timestamp() const {
 using namespace std::chrono;
 
 auto now = system_clock::now();
 auto time = system_clock::to_time_t(now);
 auto ms = duration_cast<milliseconds>(now.time_since_epoch()) % 1000;
 
 std::ostringstream oss;
 oss << std::put_time(std::localtime(&time), "%Y-%m-%d %H:%M:%S")
 << "." << std::setw(3) << std::setfill('0') << ms.count();
 return oss.str();
 }
 
 void MetricsLogger::run() {
 std::ofstream out(filename_, std::ios::app);
 while (running_) {
 auto now = get_current_timestamp();
 std::ostringstream line;
 line << now;
 
 for (auto& metric : registry_.get_all_metrics()) {
 line << " \"" << metric->get_name() << "\" " << metric->get_value_and_reset();
 }
 
 out << line.str() << std::endl;
 out.flush();
 std::this_thread::sleep_for(interval_);
 }
 }
 
 } // namespace VkInfo

metrics_registry.h:

 #ifndef METRICS_REGISTRY_H_
 #define METRICS_REGISTRY_H_
 
 #include <vector>
 #include <memory>
 #include <mutex>
 
 #include "metric.h"
 
 namespace VkInfo {
 
 /**
 * @brief Registry for managing and storing metrics.
 *
 * MetricsRegistry maintains a thread-safe collection of metrics.
 * It allows registration of new metrics and provides access to all registered metrics.
 */
 class VKINFO_API MetricsRegistry {
 public:
 /**
 * @brief Registers a new metric.
 *
 * Thread-safe method to add a metric to the registry.
 *
 * @param metric Shared pointer to the metric to register.
 */
 void register_metric(std::shared_ptr<IMetric> metric);
 
 /**
 * @brief Retrieves all registered metrics.
 *
 * Thread-safe method to get a snapshot of all metrics currently registered.
 *
 * @return Vector of shared pointers to all registered metrics.
 */
 std::vector<std::shared_ptr<IMetric>> get_all_metrics();
 
 private:
 std::vector<std::shared_ptr<IMetric>> metrics_; ///< Container holding registered metrics.
 std::mutex mutex_; ///< Mutex to protect access to metrics_.
 };
 
 } // namespace VkInfo
 
 #endif // METRICS_REGISTRY_H_

metrics_registry.cc:

 #include "../include/metrics_registry.h"
 
 namespace VkInfo {
 
 void MetricsRegistry::register_metric(std::shared_ptr<IMetric> metric) {
 std::lock_guard<std::mutex> lock(mutex_);
 metrics_.push_back(std::move(metric));
 }
 
 std::vector<std::shared_ptr<IMetric>> MetricsRegistry::get_all_metrics() {
 std::lock_guard<std::mutex> lock(mutex_);
 return metrics_;
 }
 
 } // namespace VkInfo

For complete project, refer to GitHub.

• pacmaninbw: An observation about the GitHub repository. Generally a GitHub repository, or really any git repository should not contain generated code such as .so shared libraries and executable binaries. The .gitignore file should generally exclude the build directory which contains only generated files. A git repository should only contain what is necessary to build the application. The application may be built on multiple platforms, and generated code will only work on a single platform.
• pacmaninbw: See the GitHub .gitignore examples.

Answer 1

Not a full review; I’m just going to focus on a specific topic.

As of C++20, you should consider std::thread to be effectively deprecated. In almost all cases, new code should use std::jthread. In particular, your code could really benefit from it.

I’m going to repeat your existing code, stripped down to the relevant bits:

class MetricsLogger
{
public:
 ~MetricsLogger()
 {
 stop();
 }
 void start()
 {
 if (running_) return;
 running_ = true;
 worker_ = std::thread(&MetricsLogger::run, this);
 }
 void stop()
 {
 if (!running_) return;
 running_ = false;
 if (worker_.joinable()) {
 worker_.join();
 }
 }
private:
 void run()
 {
 while (running_)
 {
 // ...
 }
 }
 std::atomic<bool> running_;
 std::thread worker_;
};

One of the improvements std::jthread brings is built-in support for cancellation. That means that virtually all of the code above is superfluous.

First you would change your run() function to take a std::stop_token, and use that to determine when to stop running:

 void run(std::stop_token stoken)
 {
 while (not stoken.stop_requested())
 {
 // ...
 }
 }

Next you would use the built-in stop request mechanism in stop():

 void stop()
 {
 if (worker_.request_stop())
 worker_ = std::jthread{}; // This will replace the
 // worker thread with a "null"
 // thread. The old worker
 // thread will be auto-joined
 // as it is destroyed.
 }

Now if you want to start the thread only if it is not already running, you just need to directly check if the thread is already running:

 void start()
 {
 if (worker_.joinable())
 return;
 worker_ = std::jthread{std::bind_front(&MetricsLogger::run, this)};
 }

Note that you need to use std::bind_front() to make this work. If you do not, then jthread will try to call run(stop_token, this); the this is in the wrong place. The bind_front() creates a new callable that has the this already bound, so jthread can call it as just <func>(stop_token), and it will be expanded as run(this, stop_token)... which is what you want.

But now you no longer need runnable_, or the destructor:

class MetricsLogger
{
public:
 void start()
 {
 if (worker_.joinable())
 return;
 worker_ = std::jthread{std::bind_front(&MetricsLogger::run, this)};
 }
 void stop()
 {
 if (worker_.request_stop())
 worker_ = std::jthread{};
 }
private:
 void run(std::stop_token stoken)
 {
 while (not stoken.stop_requested())
 {
 // ...
 }
 }
 std::jthread worker_;
};

This is not only much simpler, it also fixes some subtle bugs.

Again, look at your stop function:

 void MetricsLogger::stop() {
 if (!running_) return;
 running_ = false;
 if (worker_.joinable()) {
 worker_.join();
 }
 }

Imagine what would happen if two threads called .stop() at roughly the same time. The first thread checks running_, and sees it is true... then the thread gets preempted. The second thread checks running_ and also sees it is true. So it duly goes about its business setting running_ to false, testing worker_.joinable() and seeing it is true... and then that thread gets preempted. The first thread continues, having already seen running_ as true it sets running_ to false and tests worker_.joinable() and sees true, then calls worker_.join(). And then the second thread calls worker._join(). Boom. UB.

The problem with your code is that there are too many tests going on, and they are not atomic. Or rather, they are atomic individually, but not atomic collectively. Testing running_ is atomic (because it is an atomic bool), and testing worker_.joinable() is atomic... but both tests together are not atomic.

By contrast with the jthread version:

 void MetricsLogger::stop()
 {
 if (worker_.request_stop())
 worker_ = std::jthread{};
 }

... there is a single test, and it is atomic. If worker_.request_stop() returns true, then this thread just did the stop... so this thread can safely join the worker thread, because no other thread will. (That is, presuming that another thread is not trying to destroy the MetricsLogger object while this thread is doing .stop(). But if that is happening, you have much bigger problems.)

However, you still have another potential data race, and I do not think there is any possible way to fix it. What happens if two threads call .start() at the same time?

If two threads try calling .start() at the same time, both might see that the thread has not been started... and then both will try to start it. Assuming the move assignment is atomic (!!!), then in the original code, you will get a hard crash (technically, std::terminate()), because one worker thread will be created and assigned to worker_... then another worker thread will be created and assigned to worker_, and that will kill the program. In the jthread version, it will "work", in that one worker thread will be created and assigned to worker_... then another worker thread will be created and assigned to worker_. and that will cause a stop request and join on the first worker thread. So, basically, two worker threads will be created, but one will be immediately stopped. So, in that sense, it "works". (Although, two threads will exist, even if only for a brief moment, so it that sense it doesn’t really work.)

However, all that assumes that std::(j)thread’s move assignment operator is atomic... which I don’t know if it is. If it is not, then it may be possible for the two moves to be interleaved... and if that happens, you’re in for a massive clusterfuck, regardless of whether you’re using std::thread or std::jthread.

My advice? Avoid that quagmire altogether. Start the worker thread in the constructor. Do not provide any way to restart it. Allow stop to be called multiple times, even concurrently. But once it’s stopped, it’s stopped. No restarting.

Either that or, if you really want to have the start delayed until some time after construction, use some mechanism to ensure that .start() can only be called once... ever. For example:

class MetricsLogger
{
public:
 void start()
 {
 if (not started_.test_and_set())
 worker_ = std::jthread{std::bind_front(&MetricsLogger::run, this)};
 }
 void stop()
 {
 if (worker_.request_stop())
 worker_ = std::jthread{};
 }
private:
 std::atomic_flag started_;
 std::jthread worker_;
};

If you want to allow re-starting... well, that is a massively more difficult problem. I can’t imagine you actually NEED that functionality. And if you don’t NEED it, it’s really not worth the trouble.

Answer 2

Congratulations for writing C++ code that looks like an actual C++.

Now, a few, non-exhaustive points:

Indentation

MetricsLogger::MetricsLogger(MetricsRegistry& registry,
 const std::string& filename,
 std::chrono::milliseconds interval)
: registry_(registry),
 filename_(filename),
 interval_(interval),
 running_(false) {}

Wrong indentation here. I suppose you wanted the second line to align with MetricsRegistry of the first line, but you got one space less for that. And I have no idea what happened with registry_ and filename_. In all cases, avoid arbitrary spacing, because:

void DoSomething(int x,
 int y)

would be screwed as soon as you rename DoSomething to DifferentName:

void DifferentName(int x,
 int y)

And if you think it wouldn't, think twice: what happens when you rename a method through the IDE? Do you check spacing at every occurrence? Really?

If you caught this and adjusted the spacing, you created another problem: your diff now spans multiple lines, instead of being limited to one.

Instead, do:

MetricsLogger::MetricsLogger(
 MetricsRegistry& registry,
 const std::string& filename,
 std::chrono::milliseconds interval) :
 registry_(registry),
 filename_(filename),
 interval_(interval),
 running_(false) {}

Constants

In:

auto now = system_clock::now();
auto time = system_clock::to_time_t(now);
auto ms = duration_cast<milliseconds>(now.time_since_epoch()) % 1000;

you don't modify the variables being declared. Be explicit about it and declare them const.

Same for the instance methods that do not modify the object. Declare them const as well.

Underscore convention

I've never seen the trailing underscore (registry_) convention in C++.

Are you sure this is something popular?

Thread safety

When you do:

std::vector<std::shared_ptr<IMetric>> MetricsRegistry::get_all_metrics() {
 std::lock_guard<std::mutex> lock(mutex_);
 return metrics_;
}

what exactly are you trying to achieve with the lock?

String interpolation

In:

oss << std::put_time(std::localtime(&time), "%Y-%m-%d %H:%M:%S")
 << "." << std::setw(3) << std::setfill('0') << ms.count();

you may want to use std::format instead, in order to get the string directly, instead of having to use std::ostringstream. But it might end up with code that is less readable—in which case keep your current approach.

• indi: "I've never seen the trailing underscore (registry_) convention in C++." It is one of the most popular conventions. It is even in the Google style guide (which I generally don’t like, but it is stupidly (deliberate word choice) popular, and the point is that the trailing underscore guideline is there). The only other convention remotely close in popularity is a prefixed m_.
• pacmaninbw: Re: String interpolation, I agree that std::format would probably be better, but since they are using gcc-12 (according to the problem description on GitHub), they don't have access to the chrono portion of std:format, it isn't available until gcc-14. I had to upgrade my computer to Ubuntu 24.04 for exactly this reason. Not for this question, but a project I am working on that requires std::chrono formatting.
• SyRoN: Thanks! Really good point on indentation. Speaking about Multithreading, i guess the idea was to protect metric_registry's vector of metrics from being used by multiple threads. Is standard library vector thread safe?
• indi: "Is standard library vector thread safe?" It is not. Guarding the vector while copying it is wise.