[for upstream] mlog: Add async I/O and directory prefix support to FileMLogger

moxygen/mlog/CMakeLists.txt

@@ -50,3 +50,5 @@ moxygen_add_library(moxygen_mlog_file_mlogger_factory
     moxygen_mlog_file_mlogger
     moxygen_mlog_mlogger_factory
 )
+
+add_subdirectory(test)

moxygen/mlog/FileMLogger.cpp

@@ -5,19 +5,56 @@
  */
 
 #include "moxygen/mlog/FileMLogger.h"
+#include <folly/Random.h>
 #include <folly/json/json.h>
+#include <folly/logging/xlog.h>
 #include <fstream>
 
 namespace moxygen {
 
+std::optional<std::string> FileMLogger::derivePath() const {
+  if (dir_) {
+    if (!dcid_) {
+      XLOG(WARN) << "Skipping mlog output: dcid must be set when dir is configured";
+      return std::nullopt;
+    }
+    const auto dcidHex = dcid_->hex();
+    if (dcidHex.empty()) {
+      XLOG(WARN)
+          << "Skipping mlog output: dcid must have a non-empty hex representation when dir is configured";
+      return std::nullopt;
+    }
+    return *dir_ + "/" + dcidHex + ".mlog";
+  }
+  return path_;
+}
+
 void FileMLogger::outputLogs() {
-  std::ofstream fileObj(path_);
+  auto path = derivePath();
+  if (!path) {
+    return;
+  }
+
+  // Pre-format logs on the calling thread (where formatLog() is safe to
+  // call), then execute the write lambda either inline or via the executor.
+  std::vector<std::string> serialized;
+  serialized.reserve(logs_.size());
   for (const auto& log : logs_) {
-    auto obj = formatLog(log);
-    std::string jsonLog = folly::toPrettyJson(obj);
-    fileObj << jsonLog << std::endl;
+    serialized.push_back(folly::toPrettyJson(formatLog(log)));
+  }
+
+  auto write = [p = std::move(*path), lines = std::move(serialized)]() {
+    std::ofstream fileObj(p);
+    for (const auto& line : lines) {
+      fileObj << line << '\n';
+    }
+  };
+
+  if (writeExecutor_) {
+    writeExecutor_->add(std::move(write));
+  } else {
+    write();
   }
-  fileObj.close();
 }
 
 } // namespace moxygen

moxygen/mlog/FileMLogger.h

@@ -6,6 +6,10 @@
 
 #pragma once
 
+#include <folly/Executor.h>
+#include <memory>
+#include <optional>
+#include <string>
 #include "moxygen/mlog/MLogger.h"
 
 namespace moxygen {
@@ -14,17 +18,50 @@ namespace moxygen {
  * FileMLogger is a concrete MLogger implementation that outputs logs to a file.
  * It inherits from the abstract MLogger base class and implements outputLogs()
  * to write formatted JSON logs to the path specified via setPath().
+ *
+ * If a write executor is set via setWriteExecutor(), outputLogs() will
+ * pre-format logs on the calling thread and schedule the file write
+ * asynchronously on that executor.
+ *
+ * If a directory is set via setDir(), setPath() will prepend it automatically.
  */
 class FileMLogger : public MLogger {
  public:
   explicit FileMLogger(VantagePoint vantagePoint) : MLogger(vantagePoint) {}
   ~FileMLogger() override = default;
 
   /**
-   * Outputs all accumulated logs to the file specified by path_.
+   * Set an executor for asynchronous file writes.
+   */
+  void setWriteExecutor(std::shared_ptr<folly::Executor> executor) {
+    writeExecutor_ = std::move(executor);
+  }
+
+  /**
+   * Set an output directory
+   */
+  void setDir(std::string dir) {
+    dir_ = std::move(dir);
+  }
+
+  /**
+   * Outputs all accumulated logs to the file specified by path_ or
+   * auto-derived path (if dir is set).
    * Logs are formatted as pretty-printed JSON.
    */
   void outputLogs() override;
+
+ private:
+  /**
+     * Derives the output file path. If dir is set, dcid with a non-empty hex
+     * representation is required and the path is {dir}/{dcid_hex}.mlog.
+     * Otherwise returns the explicit path_. Returns nullopt when logging should
+     * be skipped.
+   */
+    std::optional<std::string> derivePath() const;
+
+  std::shared_ptr<folly::Executor> writeExecutor_;
+  std::optional<std::string> dir_;
 };
 
 } // namespace moxygen

moxygen/mlog/FileMLoggerFactory.h

@@ -6,27 +6,55 @@
 
 #pragma once
 
+#include <folly/Executor.h>
 #include <moxygen/mlog/FileMLogger.h>
+#include <memory>
+#include <optional>
 #include <string>
 #include "moxygen/mlog/MLoggerFactory.h"
 
 namespace moxygen {
 
-// Creates an MLogger that writes to a file
+// Creates FileMLogger instances per session.
+// Optionally forwards a write executor and/or output directory to each created
+// logger.
 class FileMLoggerFactory : public MLoggerFactory {
  public:
-  FileMLoggerFactory(const std::string& path, VantagePoint vantagePoint)
-      : path_(path), vantagePoint_(vantagePoint) {}
+  explicit FileMLoggerFactory(VantagePoint vantagePoint = VantagePoint::SERVER)
+      : vantagePoint_(vantagePoint) {}
+
+  explicit FileMLoggerFactory(
+      std::string path,
+      VantagePoint vantagePoint = VantagePoint::SERVER)
+      : vantagePoint_(vantagePoint), path_(std::move(path)) {}
 
   std::shared_ptr<MLogger> createMLogger() override {
     auto logger = std::make_shared<FileMLogger>(vantagePoint_);
-    logger->setPath(path_);
+    if (writeExecutor_) {
+      logger->setWriteExecutor(writeExecutor_);
+    }
+    if (path_) {
+      logger->setPath(*path_);
+    }
+    if (dir_) {
+      logger->setDir(*dir_);
+    }
     return logger;
   }
 
+  void setWriteExecutor(std::shared_ptr<folly::Executor> executor) {
+    writeExecutor_ = std::move(executor);
+  }
+
+  void setDir(std::string dir) {
+    dir_ = std::move(dir);
+  }
+
  private:
-  std::string path_;
   VantagePoint vantagePoint_;
+  std::shared_ptr<folly::Executor> writeExecutor_;
+  std::optional<std::string> path_;
+  std::optional<std::string> dir_;
 };
 
 } // namespace moxygen

moxygen/mlog/test/CMakeLists.txt

@@ -0,0 +1,20 @@
+# Copyright (c) Meta Platforms, Inc. and affiliates.
+# This source code is licensed under the Apache 2.0 license found in the
+# LICENSE file in the root directory of this source tree.
+
+if(NOT BUILD_TESTS)
+    return()
+endif()
+
+moxygen_add_test(TARGET MLoggerTests
+  PREFIX "MLogger"
+  SOURCES
+    FileMLoggerTest.cpp
+    FileMLoggerFactoryTest.cpp
+  DEPENDS
+    moxygen::moxygen_mlog_file_mlogger
+    moxygen::moxygen_mlog_file_mlogger_factory
+    Folly::folly_executors_manual_executor
+    mvfst::mvfst_codec_types
+    GTest::gtest_main
+)

moxygen/mlog/test/FileMLoggerFactoryTest.cpp

@@ -0,0 +1,146 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ * This source code is licensed under the Apache 2.0 license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+#include <moxygen/mlog/FileMLogger.h>
+#include <moxygen/mlog/FileMLoggerFactory.h>
+
+#include <folly/executors/ManualExecutor.h>
+#include <folly/portability/GTest.h>
+#include <quic/codec/QuicConnectionId.h>
+#include <filesystem>
+
+namespace fs = std::filesystem;
+
+namespace moxygen {
+
+class FileMLoggerFactoryTest : public ::testing::Test {
+ protected:
+  void SetUp() override {
+    dir_ = fs::path(testing::TempDir()) / "mlog_factory_test";
+    fs::create_directories(dir_);
+  }
+
+  void TearDown() override {
+    fs::remove_all(dir_);
+  }
+
+  fs::path dir_;
+};
+
+// ---------------------------------------------------------------------------
+// Constructor tests
+// ---------------------------------------------------------------------------
+
+TEST_F(FileMLoggerFactoryTest, PathCtor_CreatesLoggerThatWritesToGivenPath) {
+  const auto kFactoryOutputFile = "factory_out.mlog";
+  auto path = (dir_ / kFactoryOutputFile).string();
+  FileMLoggerFactory factory(path);
+
+  auto logger = factory.createMLogger();
+  ASSERT_NE(logger, nullptr);
+
+  logger->outputLogs();
+  EXPECT_TRUE(fs::exists(path));
+}
+
+// ---------------------------------------------------------------------------
+// setDir propagation
+// ---------------------------------------------------------------------------
+
+// Factory propagates dir to loggers: outputLogs() should use dcid-derived filename
+TEST_F(FileMLoggerFactoryTest, SetDir_PropagatesDirToLogger) {
+  const auto kFile = "11223344.mlog";
+  const std::vector<uint8_t> kCid = {0x11, 0x22, 0x33, 0x44};
+  FileMLoggerFactory factory;
+  factory.setDir(dir_.string());
+
+  auto logger = factory.createMLogger();
+  ASSERT_NE(logger, nullptr);
+
+  // Give the logger a known dcid so derivePath produces a predictable filename
+  logger->setDcid(
+      quic::ConnectionId::createAndMaybeCrash(kCid));
+  logger->outputLogs();
+
+  EXPECT_TRUE(fs::exists(dir_ / kFile));
+}
+
+// Multiple loggers from the same factory each get their own dir
+TEST_F(FileMLoggerFactoryTest, SetDir_EachLoggerGetsOwnFile) {
+  const auto kLoggerAFile = "55667788.mlog";
+  const auto kLoggerBFile = "99aabbcc.mlog";
+  const std::vector<uint8_t> kLoggerACid = {0x55, 0x66, 0x77, 0x88};
+  const std::vector<uint8_t> kLoggerBCid = {0x99, 0xAA, 0xBB, 0xCC};
+  FileMLoggerFactory factory;
+  factory.setDir(dir_.string());
+
+  auto loggerA = factory.createMLogger();
+  auto loggerB = factory.createMLogger();
+  ASSERT_NE(loggerA, nullptr);
+  ASSERT_NE(loggerB, nullptr);
+
+  loggerA->setDcid(quic::ConnectionId::createAndMaybeCrash(kLoggerACid));
+  loggerB->setDcid(quic::ConnectionId::createAndMaybeCrash(kLoggerBCid));
+  loggerA->outputLogs();
+  loggerB->outputLogs();
+
+  EXPECT_TRUE(fs::exists(dir_ / kLoggerAFile));
+  EXPECT_TRUE(fs::exists(dir_ / kLoggerBFile));
+}
+
+// ---------------------------------------------------------------------------
+// setWriteExecutor propagation
+// ---------------------------------------------------------------------------
+
+TEST_F(FileMLoggerFactoryTest, SetWriteExecutor_PropagatesAsyncBehavior) {
+  const auto kAsyncFactoryFile = "async_factory.mlog";
+  auto executor = std::make_shared<folly::ManualExecutor>();
+  auto path = (dir_ / kAsyncFactoryFile).string();
+
+  FileMLoggerFactory factory(path);
+  factory.setWriteExecutor(executor);
+
+  auto logger = factory.createMLogger();
+  ASSERT_NE(logger, nullptr);
+
+  logger->outputLogs();
+  // Not yet written — executor not drained
+  EXPECT_FALSE(fs::exists(path));
+
+  executor->drain();
+  EXPECT_TRUE(fs::exists(path));
+}
+
+// Each logger created by the factory shares the same executor
+TEST_F(FileMLoggerFactoryTest, SetWriteExecutor_SharedAcrossLoggers) {
+  const auto kAsyncLoggerAFile = "ddeeff00.mlog";
+  const auto kAsyncLoggerBFile = "aabbccdd.mlog";
+  const std::vector<uint8_t> kAsyncLoggerACid = {0xDD, 0xEE, 0xFF, 0x00};
+  const std::vector<uint8_t> kAsyncLoggerBCid = {0xAA, 0xBB, 0xCC, 0xDD};
+  auto executor = std::make_shared<folly::ManualExecutor>();
+  FileMLoggerFactory factory;
+  factory.setDir(dir_.string());
+  factory.setWriteExecutor(executor);
+
+  auto loggerA = factory.createMLogger();
+  auto loggerB = factory.createMLogger();
+
+  loggerA->setDcid(quic::ConnectionId::createAndMaybeCrash(kAsyncLoggerACid));
+  loggerB->setDcid(quic::ConnectionId::createAndMaybeCrash(kAsyncLoggerBCid));
+
+  loggerA->outputLogs();
+  loggerB->outputLogs();
+
+  EXPECT_FALSE(fs::exists(dir_ / kAsyncLoggerAFile));
+  EXPECT_FALSE(fs::exists(dir_ / kAsyncLoggerBFile));
+
+  executor->drain();
+
+  EXPECT_TRUE(fs::exists(dir_ / kAsyncLoggerAFile));
+  EXPECT_TRUE(fs::exists(dir_ / kAsyncLoggerBFile));
+}
+
+} // namespace moxygen