Skip to content

Commit 6f133cf

Browse files
Chandhana Solainathanmeta-codesync[bot]
authored andcommitted
telemetry: Instrument NFS error paths with structured error logging
Summary: Logs NFS SERVERFAULT errors to perfpipe_edenfs_errors across all NFS RPC handlers. Only unexpected failures (SERVERFAULT) are logged. Reviewed By: vilatto Differential Revision: D105984136 fbshipit-source-id: c46d403495a04d998847264c99a7f8cfb33b822d
1 parent 61b5ab6 commit 6f133cf

7 files changed

Lines changed: 319 additions & 81 deletions

File tree

eden/fs/nfs/BUCK

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,8 @@ cpp_library(
5151
"//eden/common/utils:utils",
5252
"//eden/fs/privhelper:interface",
5353
"//eden/fs/store:context",
54+
"//eden/fs/telemetry:error_info",
55+
"//eden/fs/telemetry:error_logger",
5456
"//eden/fs/telemetry:fs_event_logger",
5557
"//eden/fs/telemetry:log_info",
5658
"//eden/fs/utils:clock",
@@ -70,6 +72,7 @@ cpp_library(
7072
"//eden/fs/nfs/rpc:server",
7173
"//eden/fs/telemetry:structured_logger",
7274
"//eden/fs/utils:process_access_log",
75+
"//folly:exception_wrapper",
7376
"//folly:function",
7477
],
7578
)

eden/fs/nfs/Nfsd3.cpp

Lines changed: 187 additions & 81 deletions
Large diffs are not rendered by default.

eden/fs/nfs/Nfsd3.h

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
#include <optional>
1414

15+
#include <folly/ExceptionWrapper.h>
1516
#include "eden/common/telemetry/TraceBus.h"
1617
#include "eden/common/utils/CaseSensitivity.h"
1718
#include "eden/fs/inodes/FsChannel.h"
@@ -33,6 +34,19 @@ class ProcessInfoCache;
3334
class EdenFsEventsLogger;
3435
class FsEventLogger;
3536

37+
namespace detail {
38+
/**
39+
* Log SERVERFAULT errors to structured error telemetry.
40+
* Normal filesystem errors (ENOENT, EACCES, etc.) are not logged.
41+
*/
42+
void logNfsError(
43+
nfsstat3 error,
44+
const folly::exception_wrapper& ex,
45+
ErrorLogger& errorLogger,
46+
uint64_t inode,
47+
const AbsolutePath& mountPath);
48+
} // namespace detail
49+
3650
using TraceDetailedArgumentsHandle = std::shared_ptr<void>;
3751

3852
enum class NfsInvalidationSource : uint8_t {

eden/fs/nfs/test/BUCK

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,14 @@ cpp_unittest(
88
supports_static_listing = False,
99
deps = [
1010
"fbsource//third-party/googletest:gtest",
11+
"//eden/common/utils:path",
12+
"//eden/fs/config:config",
1113
"//eden/fs/nfs:dirlist",
1214
"//eden/fs/nfs:nfs_utils",
15+
"//eden/fs/nfs:nfsd3",
1316
"//eden/fs/nfs:nfsd_rpc",
1417
"//eden/fs/nfs/testharness:xdr_test_utils",
18+
"//eden/fs/telemetry:error_logger",
19+
"//eden/fs/telemetry/test:capturing_scribe_logger",
1520
],
1621
)
Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
/*
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* This software may be used and distributed according to the terms of the
5+
* GNU General Public License version 2.
6+
*/
7+
8+
#include <gtest/gtest.h>
9+
#include <stdexcept>
10+
#include <system_error>
11+
12+
#include "eden/common/utils/PathFuncs.h"
13+
#include "eden/fs/config/EdenConfig.h"
14+
#include "eden/fs/config/ReloadableConfig.h"
15+
#include "eden/fs/nfs/Nfsd3.h"
16+
#include "eden/fs/telemetry/ErrorLogger.h"
17+
#include "eden/fs/telemetry/test/CapturingScribeLogger.h"
18+
19+
using namespace facebook::eden;
20+
21+
namespace {
22+
23+
std::shared_ptr<ReloadableConfig> makeTestConfig() {
24+
auto edenConfig = EdenConfig::createTestEdenConfig();
25+
edenConfig->enableErrorLogging.setValue(
26+
true, ConfigSourceType::Default, true);
27+
return std::make_shared<ReloadableConfig>(edenConfig);
28+
}
29+
30+
} // namespace
31+
32+
TEST(NfsErrorLoggingTest, serverfaultIsLogged) {
33+
auto scribe = std::make_shared<CapturingScribeLogger>();
34+
auto config = makeTestConfig();
35+
ErrorLogger logger(scribe, SessionInfo{}, config);
36+
37+
folly::exception_wrapper ex{std::runtime_error("backing store failure")};
38+
detail::logNfsError(
39+
nfsstat3::NFS3ERR_SERVERFAULT,
40+
ex,
41+
logger,
42+
42,
43+
canonicalPath("/mnt/repo"));
44+
45+
ASSERT_EQ(scribe->messages().size(), 1);
46+
const auto& msg = scribe->messages()[0];
47+
EXPECT_NE(msg.find("nfs"), std::string::npos)
48+
<< "Should contain nfs component, got: " << msg;
49+
EXPECT_NE(msg.find("backing store failure"), std::string::npos)
50+
<< "Should contain error message, got: " << msg;
51+
EXPECT_NE(msg.find("mnt"), std::string::npos)
52+
<< "Should contain mount point, got: " << msg;
53+
}
54+
55+
TEST(NfsErrorLoggingTest, nonServerfaultIsNotLogged) {
56+
auto scribe = std::make_shared<CapturingScribeLogger>();
57+
auto config = makeTestConfig();
58+
ErrorLogger logger(scribe, SessionInfo{}, config);
59+
60+
folly::exception_wrapper ex{
61+
std::system_error(ENOENT, std::generic_category(), "file not found")};
62+
detail::logNfsError(
63+
nfsstat3::NFS3ERR_NOENT, ex, logger, 42, canonicalPath("/mnt/repo"));
64+
65+
EXPECT_EQ(scribe->messages().size(), 0)
66+
<< "Non-SERVERFAULT errors should not be logged";
67+
}

eden/fs/telemetry/test/BUCK

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,17 @@
11
load("@fbcode_macros//build_defs:cpp_benchmark.bzl", "cpp_benchmark")
2+
load("@fbcode_macros//build_defs:cpp_library.bzl", "cpp_library")
23
load("@fbcode_macros//build_defs:cpp_unittest.bzl", "cpp_unittest")
34

45
oncall("scm_client_infra")
56

7+
cpp_library(
8+
name = "capturing_scribe_logger",
9+
headers = ["CapturingScribeLogger.h"],
10+
exported_deps = [
11+
"//eden/common/telemetry:scribe_logger",
12+
],
13+
)
14+
615
cpp_unittest(
716
name = "task_trace_test",
817
srcs = [
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
/*
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* This software may be used and distributed according to the terms of the
5+
* GNU General Public License version 2.
6+
*/
7+
8+
#pragma once
9+
10+
#include <string>
11+
#include <vector>
12+
13+
#include "eden/common/telemetry/ScribeLogger.h"
14+
15+
namespace facebook::eden {
16+
17+
/**
18+
* A ScribeLogger that captures messages for test verification.
19+
*/
20+
class CapturingScribeLogger : public ScribeLogger {
21+
public:
22+
void log(std::string message) override {
23+
messages_.push_back(std::move(message));
24+
}
25+
26+
const std::vector<std::string>& messages() const {
27+
return messages_;
28+
}
29+
30+
private:
31+
std::vector<std::string> messages_;
32+
};
33+
34+
} // namespace facebook::eden

0 commit comments

Comments
 (0)