Skip to content

Commit

Permalink
add the notion of "typeless" events
Browse files Browse the repository at this point in the history
Summary:
"type" is a column that exists in some of our datasets, but right now the use of "type" restricts a more freeform table (if a logger only logs one event, type doesn't really make sense). This diff adds a "typeless" log method, where the StructuredLogger can log Dynamic events without a `type`

I was debating if I should make this a `std::optional` or just use a `nullptr` for typeless events. It looks to be minimally different after compilation, so I opted for the `std::optional` for readability/safety sake

`std::optional`: https://godbolt.org/z/Ms6EqMMKh

`nullptr`: https://godbolt.org/z/6YTd9ax3M

Reviewed By: jdelliot

Differential Revision: D63797398

fbshipit-source-id: a92e4e846e6305b89fcf696c24f6b83d7f1966be
  • Loading branch information
genevievehelsel authored and facebook-github-bot committed Oct 10, 2024
1 parent 5842626 commit 96640dd
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 3 deletions.
11 changes: 11 additions & 0 deletions eden/common/telemetry/LogEvent.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@ struct TypedEvent {
virtual const char* getType() const = 0;
};

struct TypelessEvent {
virtual ~TypelessEvent() = default;
virtual void populate(DynamicEvent&) const = 0;
};

// Used for unit testing
struct TestEvent : public TypedEvent {
// Keep populate() and getType() pure virtual to force subclasses
Expand All @@ -27,4 +32,10 @@ struct TestEvent : public TypedEvent {
virtual const char* getType() const override = 0;
};

// Used for unit testing
struct TypelessTestEvent : public TypelessEvent {
// Keep populate() pure virtual to force subclasses to implement it
virtual void populate(DynamicEvent&) const override = 0;
};

} // namespace facebook::eden
7 changes: 5 additions & 2 deletions eden/common/telemetry/StructuredLogger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,16 @@ StructuredLogger::StructuredLogger(bool enabled, SessionInfo sessionInfo)
sessionId_{getSessionId()},
sessionInfo_{std::move(sessionInfo)} {}

DynamicEvent StructuredLogger::populateDefaultFields(const char* type) {
DynamicEvent StructuredLogger::populateDefaultFields(
std::optional<const char*> type) {
DynamicEvent event;
if (kExplicitTimeField) {
event.addInt("time", ::time(nullptr));
}
event.addInt("session_id", sessionId_);
event.addString("type", type);
if (type.has_value()) {
event.addString("type", *type);
}
event.addString("user", sessionInfo_.username);
event.addString("host", sessionInfo_.hostname);
event.addString("os", sessionInfo_.os);
Expand Down
14 changes: 13 additions & 1 deletion eden/common/telemetry/StructuredLogger.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,18 @@ class StructuredLogger {
explicit StructuredLogger(bool enabled, SessionInfo sessionInfo);
virtual ~StructuredLogger() = default;

void logEvent(const TypelessEvent& event) {
// Avoid a bunch of work if it's going to be thrown away by the
// logDynamicEvent implementation.
if (!enabled_) {
return;
}

DynamicEvent de{populateDefaultFields(std::nullopt)};
event.populate(de);
logDynamicEvent(std::move(de));
}

void logEvent(const TypedEvent& event) {
// Avoid a bunch of work if it's going to be thrown away by the
// logDynamicEvent implementation.
Expand All @@ -35,7 +47,7 @@ class StructuredLogger {
protected:
virtual void logDynamicEvent(DynamicEvent event) = 0;

virtual DynamicEvent populateDefaultFields(const char* type);
virtual DynamicEvent populateDefaultFields(std::optional<const char*> type);

bool enabled_;
uint32_t sessionId_;
Expand Down
42 changes: 42 additions & 0 deletions eden/common/telemetry/test/ScubaStructuredLoggerTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,19 @@ struct TestLogEvent : public TestEvent {
}
};

struct TypelessTestLogEvent : public TypelessTestEvent {
std::string str;
int number = 0;

TypelessTestLogEvent(std::string str, int number)
: str(std::move(str)), number(number) {}

void populate(DynamicEvent& event) const override {
event.addString("str", str);
event.addInt("number", number);
}
};

struct ScubaStructuredLoggerTest : public ::testing::Test {
std::shared_ptr<TestScribeLogger> scribe{
std::make_shared<TestScribeLogger>()};
Expand Down Expand Up @@ -96,3 +109,32 @@ TEST_F(ScubaStructuredLoggerTest, json_contains_types_at_top_level_and_values) {
UnorderedElementsAre("str", "user", "host", "type", "os", "osver"));
#endif
}

TEST_F(
ScubaStructuredLoggerTest,
typeless_json_doesnt_contain_type_at_top_level) {
logger.logEvent(TypelessTestLogEvent{"different name", 12});
EXPECT_EQ(1, scribe->lines.size());
const auto& line = scribe->lines[0];
auto doc = folly::parseJson(line);
EXPECT_TRUE(doc.isObject());
EXPECT_THAT(keysOf(doc), UnorderedElementsAre("int", "normal"));

auto ints = doc["int"];
EXPECT_TRUE(ints.isObject());
EXPECT_THAT(
keysOf(ints), UnorderedElementsAre("time", "number", "session_id"));

auto normals = doc["normal"];
EXPECT_TRUE(normals.isObject());
#if defined(__APPLE__)
EXPECT_THAT(
keysOf(normals),
UnorderedElementsAre(
"str", "user", "host", "os", "osver", "system_architecture"));
#else
EXPECT_THAT(
keysOf(normals),
UnorderedElementsAre("str", "user", "host", "os", "osver"));
#endif
}

0 comments on commit 96640dd

Please sign in to comment.