Skip to content

Commit

Permalink
Refactor EdenStats, DurationScope and StatsGroup
Browse files Browse the repository at this point in the history
Summary:
To support better telemetry and logging in watchman we want to use Eden's components. Lets migrate and detangle the needed pieces.

This change refactors EdenStas, DurationScope and StatsGroup..

Reviewed By: kmancini

Differential Revision: D54566459

fbshipit-source-id: 8946dbdf3ca9ddbc5c30b070d305ef499541ea3a
  • Loading branch information
jdelliot authored and facebook-github-bot committed Mar 8, 2024
1 parent 499091e commit 2ee07b6
Show file tree
Hide file tree
Showing 24 changed files with 248 additions and 172 deletions.
1 change: 1 addition & 0 deletions eden/fs/fuse/TARGETS
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ cpp_library(
"//eden/fs/inodes:request_context",
"//eden/fs/store:context",
"//eden/fs/store:store",
"//eden/fs/telemetry:stats",
"//eden/fs/telemetry:telemetry",
"//eden/fs/utils:bufvec",
"//eden/fs/utils:fs_channel_types",
Expand Down
2 changes: 1 addition & 1 deletion eden/fs/fuse/fuse_tester/TARGETS
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ cpp_binary(
"//eden/fs/privhelper:interface",
"//eden/fs/privhelper:privhelper",
"//eden/fs/store:store",
"//eden/fs/telemetry:telemetry",
"//eden/fs/telemetry:stats",
"//eden/fs/utils:user_info",
"//folly:exception",
"//folly/init:init",
Expand Down
2 changes: 1 addition & 1 deletion eden/fs/fuse/test/TARGETS
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ cpp_unittest(
"//eden/common/utils:enum",
"//eden/common/utils:process_info_cache",
"//eden/fs/fuse:fuse",
"//eden/fs/telemetry:telemetry",
"//eden/fs/telemetry:stats",
"//eden/fs/testharness:fake_fuse",
"//eden/fs/testharness:test_dispatcher",
"//folly:random",
Expand Down
2 changes: 2 additions & 0 deletions eden/fs/inodes/TARGETS
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ cpp_library(
],
exported_deps = [
"//eden/fs/store:context",
"//eden/fs/telemetry:stats",
"//eden/fs/telemetry:telemetry",
"//eden/fs/utils:process_access_log",
"//folly/futures:core",
Expand Down Expand Up @@ -222,6 +223,7 @@ cpp_library(
"//eden/fs/takeover:takeover",
"//eden/fs/telemetry:activity_buffer",
"//eden/fs/telemetry:activity_recorder",
"//eden/fs/telemetry:stats",
"//eden/fs/telemetry:structured_logger",
"//eden/fs/telemetry:telemetry",
"//eden/fs/utils:bufvec",
Expand Down
2 changes: 1 addition & 1 deletion eden/fs/inodes/lmdbcatalog/test/TARGETS
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ cpp_unittest(
"//eden/fs/inodes/lmdbcatalog:lmdb_inode_catalog",
"//eden/fs/inodes/overlay:serialization-cpp2-types",
"//eden/fs/inodes/test:overlay_test_util",
"//eden/fs/telemetry:stats",
"//eden/fs/telemetry:structured_logger",
"//eden/fs/telemetry:telemetry",
"//eden/fs/testharness:fake_backing_store_and_tree_builder",
"//eden/fs/testharness:test_mount",
"//folly/portability:gtest",
Expand Down
2 changes: 1 addition & 1 deletion eden/fs/inodes/sqlitecatalog/test/TARGETS
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ cpp_unittest(
"//eden/fs/inodes/sqlitecatalog:bufferedsqliteinodecatalog",
"//eden/fs/inodes/sqlitecatalog:sqliteinodecatalog",
"//eden/fs/inodes/test:overlay_test_util",
"//eden/fs/telemetry:stats",
"//eden/fs/telemetry:structured_logger",
"//eden/fs/telemetry:telemetry",
"//eden/fs/testharness:fake_backing_store_and_tree_builder",
"//eden/fs/testharness:test_checks",
"//eden/fs/testharness:test_mount",
Expand Down
12 changes: 6 additions & 6 deletions eden/fs/inodes/test/TARGETS
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ cpp_unittest(
supports_static_listing = False,
deps = [
"//eden/fs/inodes:inodes",
"//eden/fs/telemetry:telemetry",
"//eden/fs/telemetry:stats",
"//folly/chrono:conv",
"//folly/experimental:test_util",
"//folly/portability:gtest",
Expand Down Expand Up @@ -318,8 +318,8 @@ cpp_unittest(
"//eden/fs/inodes/fscatalog:fsinodecatalog",
"//eden/fs/model:testutil",
"//eden/fs/service:pretty_printers",
"//eden/fs/telemetry:stats",
"//eden/fs/telemetry:structured_logger",
"//eden/fs/telemetry:telemetry",
"//eden/fs/testharness:fake_backing_store_and_tree_builder",
"//eden/fs/testharness:test_checks",
"//eden/fs/testharness:test_mount",
Expand Down Expand Up @@ -347,8 +347,8 @@ cpp_unittest(
"//eden/common/testharness:temp_file",
"//eden/fs/inodes:inodes",
"//eden/fs/inodes/fscatalog:fsinodecatalog",
"//eden/fs/telemetry:stats",
"//eden/fs/telemetry:structured_logger",
"//eden/fs/telemetry:telemetry",
"//eden/fs/testharness:test_util",
"//folly:exception",
"//folly:expected",
Expand Down Expand Up @@ -501,8 +501,8 @@ cpp_binary(
"//eden/common/utils:case_sensitivity",
"//eden/fs/config:config",
"//eden/fs/inodes:inodes",
"//eden/fs/telemetry:stats",
"//eden/fs/telemetry:structured_logger",
"//eden/fs/telemetry:telemetry",
"//folly/init:init",
"//folly/portability:gflags",
],
Expand All @@ -514,8 +514,8 @@ cpp_benchmark(
deps = [
"//eden/fs/config:config",
"//eden/fs/inodes:inodes",
"//eden/fs/telemetry:stats",
"//eden/fs/telemetry:structured_logger",
"//eden/fs/telemetry:telemetry",
"//folly:stop_watch",
"//folly/init:init",
"//folly/portability:gflags",
Expand All @@ -529,8 +529,8 @@ cpp_benchmark(
"//eden/fs/config:config",
"//eden/fs/inodes:inode_catalog",
"//eden/fs/inodes:inodes",
"//eden/fs/telemetry:stats",
"//eden/fs/telemetry:structured_logger",
"//eden/fs/telemetry:telemetry",
"//folly:stop_watch",
"//folly/init:init",
"//folly/portability:gflags",
Expand Down
2 changes: 1 addition & 1 deletion eden/fs/journal/TARGETS
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ cpp_library(
"//eden/common/utils:path",
"//eden/fs/model:model",
"//eden/fs/service:thrift-streaming-cpp2-types",
"//eden/fs/telemetry:telemetry",
"//eden/fs/telemetry:stats",
"//folly:function",
"//folly:synchronized",
],
Expand Down
2 changes: 1 addition & 1 deletion eden/fs/nfs/TARGETS
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ cpp_library(
srcs = ["NfsDispatcher.cpp"],
headers = ["NfsDispatcher.h"],
deps = [
"//eden/fs/telemetry:telemetry",
"//eden/fs/telemetry:stats",
],
exported_deps = [
":dirlist",
Expand Down
1 change: 1 addition & 0 deletions eden/fs/prjfs/TARGETS
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ cpp_library(
"//eden/fs/config:config",
"//eden/fs/notifications:notifier",
"//eden/fs/telemetry:log_info",
"//eden/fs/telemetry:stats",
"//eden/fs/telemetry:structured_logger",
"//eden/fs/utils:eden_error",
"//eden/fs/utils:static_assert",
Expand Down
1 change: 1 addition & 0 deletions eden/fs/service/TARGETS
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,7 @@ cpp_library(
"//eden/fs/takeover:takeover",
"//eden/fs/telemetry:activity_buffer",
"//eden/fs/telemetry:activity_recorder",
"//eden/fs/telemetry:stats",
"//eden/fs/telemetry:telemetry",
"//fb303:base_service",
"//folly:executor",
Expand Down
8 changes: 4 additions & 4 deletions eden/fs/store/TARGETS
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ cpp_binary(
"//eden/fs/config:config",
"//eden/fs/service:init",
"//eden/fs/service:server",
"//eden/fs/telemetry:stats",
"//eden/fs/telemetry:structured_logger",
"//eden/fs/telemetry:telemetry",
"//eden/fs/utils:user_info",
"//folly:range",
"//folly:stop_watch",
Expand All @@ -46,9 +46,9 @@ cpp_library(
"//eden/common/utils:throw",
"//eden/fs/config:config",
"//eden/fs/telemetry:log_info",
"//eden/fs/telemetry:stats",
"//eden/fs/telemetry:structured_logger",
"//eden/fs/telemetry:task_trace",
"//eden/fs/telemetry:telemetry",
"//fb303:service_data",
"//folly:string",
"//folly/futures:core",
Expand All @@ -75,7 +75,7 @@ cpp_library(
srcs = SQLITE_SRCS,
headers = SQLITE_HEADERS,
deps = [
"//eden/fs/telemetry:telemetry",
"//eden/fs/telemetry:stats",
"//folly:string",
"//folly/container:array",
"//folly/logging:logging",
Expand Down Expand Up @@ -204,7 +204,7 @@ cpp_library(
"//eden/fs/model:model",
"//eden/fs/model:model-fwd",
"//eden/fs/service:thrift-cpp2-types",
"//eden/fs/telemetry:telemetry",
"//eden/fs/telemetry:stats",
"//folly:cancellation_token",
"//folly:executor",
"//folly:intrusive_list",
Expand Down
2 changes: 1 addition & 1 deletion eden/fs/store/filter/test/TARGETS
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ cpp_unittest(
"//eden/fs/store:store",
"//eden/fs/store/filter:filtered_object_id",
"//eden/fs/store/filter:glob_filter",
"//eden/fs/telemetry:stats",
"//eden/fs/telemetry:structured_logger",
"//eden/fs/telemetry:telemetry",
"//eden/fs/testharness:fake_backing_store_and_tree_builder",
"//folly:range",
"//folly/executors:manual_executor",
Expand Down
3 changes: 2 additions & 1 deletion eden/fs/store/hg/TARGETS
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ cpp_library(
"//eden/fs/config:config",
"//eden/fs/service:thrift_util",
"//eden/fs/telemetry:log_info",
"//eden/fs/telemetry:stats",
"//eden/fs/telemetry:structured_logger",
"//eden/fs/utils:static_assert",
"//folly:executor",
Expand Down Expand Up @@ -136,7 +137,7 @@ cpp_library(
"fbsource//third-party/fmt:fmt",
"//eden/common/utils:throw",
"//eden/common/utils:utils",
"//eden/fs/telemetry:telemetry",
"//eden/fs/telemetry:stats",
"//folly/logging:logging",
],
exported_deps = [
Expand Down
1 change: 1 addition & 0 deletions eden/fs/store/hg/test/TARGETS
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ cpp_unittest(
"//eden/fs/store/hg:hg_import_request_queue",
"//eden/fs/store/hg:hg_proxy_hash",
"//eden/fs/store/hg:hg_queued_backing_store",
"//eden/fs/telemetry:stats",
"//eden/fs/telemetry:structured_logger",
"//eden/fs/telemetry:telemetry",
"//eden/fs/testharness:hg_repo",
Expand Down
4 changes: 2 additions & 2 deletions eden/fs/store/test/TARGETS
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ cpp_unittest(
"//eden/fs/store:rocksdb",
"//eden/fs/store:sqlite",
"//eden/fs/store:store",
"//eden/fs/telemetry:stats",
"//eden/fs/telemetry:structured_logger",
"//eden/fs/telemetry:telemetry",
"//eden/fs/testharness:fake_backing_store_and_tree_builder",
"//eden/fs/testharness:logging_fetch_context",
"//eden/fs/testharness:stored_object",
Expand Down Expand Up @@ -115,7 +115,7 @@ cpp_binary(
"//eden/common/utils/benchharness:benchharness",
"//eden/fs/model:model",
"//eden/fs/store:rocksdb",
"//eden/fs/telemetry:stats",
"//eden/fs/telemetry:structured_logger",
"//eden/fs/telemetry:telemetry",
],
)
72 changes: 72 additions & 0 deletions eden/fs/telemetry/DurationScope.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/*
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This software may be used and distributed according to the terms of the
* GNU General Public License version 2.
*/

#pragma once

#include <chrono>
#include <memory>

#include <folly/logging/xlog.h>
#include <folly/stop_watch.h>

#include "eden/fs/telemetry/StatsGroup.h"

namespace facebook::eden {

/**
* On construction, notes the current time. On destruction, records the elapsed
* time in the specified Stats Duration.
*
* Moveable, but not copyable.
*/
template <typename Stats>
class DurationScope {
public:
using StatsPtr = RefPtr<Stats>;

DurationScope() = delete;

template <typename T>
DurationScope(StatsPtr&& stats, StatsGroupBase::Duration T::*duration)
: stats_{std::move(stats)},
// This use of std::function won't allocate on libstdc++,
// libc++, or Microsoft STL. All three have a couple pointers
// worth of small buffer inline storage.
updateScope_{[duration](Stats& stats, StopWatch::duration elapsed) {
stats.addDuration(duration, elapsed);
}} {
assert(stats_);
}

template <typename T>
DurationScope(const StatsPtr& stats, StatsGroupBase::Duration T::*duration)
: DurationScope{stats.copy(), duration} {}

~DurationScope() noexcept {
if (stats_ && updateScope_) {
try {
updateScope_(*stats_, stopWatch_.elapsed());
} catch (const std::exception& e) {
XLOG(ERR) << "error recording duration: " << e.what();
}
}
}

DurationScope(DurationScope&& that) = default;
DurationScope& operator=(DurationScope&& that) = default;

DurationScope(const DurationScope&) = delete;
DurationScope& operator=(const DurationScope&) = delete;

private:
using StopWatch = folly::stop_watch<>;
StopWatch stopWatch_;
StatsPtr stats_;
std::function<void(Stats& stats, StopWatch::duration)> updateScope_;
};

} // namespace facebook::eden
31 changes: 0 additions & 31 deletions eden/fs/telemetry/EdenStats.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

#include "eden/fs/telemetry/EdenStats.h"

#include <chrono>
#include <memory>

namespace facebook::eden {
Expand All @@ -20,34 +19,4 @@ void EdenStats::flush() {
fb303::ServiceData::get()->getQuantileStatMap()->flushAll();
}

StatsGroupBase::Counter::Counter(std::string_view name)
: Stat{
name,
fb303::ExportTypeConsts::kSumCountAvgRate,
// Don't record quantiles for counters. Usually "1" is the only value
// added. Usually we care about counts and rates.
{},
fb303::SlidingWindowPeriodConsts::kOneMinTenMinHour,
} {
// TODO: enforce the name matches the StatsGroup prefix.
}

StatsGroupBase::Duration::Duration(std::string_view name)
: Stat{
name,
fb303::ExportTypeConsts::kSumCountAvgRate,
fb303::QuantileConsts::kP1_P10_P50_P90_P99,
fb303::SlidingWindowPeriodConsts::kOneMinTenMinHour} {
// This should be a compile-time check but I don't know how to spell that in a
// convenient way. :) Asserting at startup in debug mode should be sufficient.
XCHECK_GT(name.size(), size_t{3}) << "duration name too short";
XCHECK_EQ("_us", std::string_view(name.data() + name.size() - 3, 3))
<< "duration stats must end in _us";
// TODO: enforce the name matches the StatsGroup prefix.
}

void StatsGroupBase::Duration::addDuration(std::chrono::microseconds elapsed) {
addValue(elapsed.count());
}

} // namespace facebook::eden
Loading

0 comments on commit 2ee07b6

Please sign in to comment.