Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 93 additions & 0 deletions lldb/include/lldb/Core/Telemetry.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
//===-- Telemetry.h ----------------------------------------------*- C++
//-*-===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//

#ifndef LLDB_CORE_TELEMETRY_H
#define LLDB_CORE_TELEMETRY_H

#include "lldb/Core/StructuredDataImpl.h"
#include "lldb/Interpreter/CommandReturnObject.h"
#include "lldb/Utility/StructuredData.h"
#include "lldb/lldb-forward.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/JSON.h"
#include "llvm/Telemetry/Telemetry.h"
#include <chrono>
#include <ctime>
#include <memory>
#include <optional>
#include <string>
#include <unordered_map>

namespace lldb_private {

struct LldbEntryKind : public ::llvm::telemetry::EntryKind {
static const llvm::telemetry::KindType BaseInfo = 0b11000;
};

/// Defines a convenient type for timestamp of various events.
/// This is used by the EventStats below.
using SteadyTimePoint = std::chrono::time_point<std::chrono::steady_clock,
std::chrono::nanoseconds>;

/// Various time (and possibly memory) statistics of an event.
struct EventStats {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[begin bikeshedding]
Given the current implementation, I don't think there are "stats". How about just EventData or Data. IIUC, this class is meant to represent data that all the LLDB TelemetryInfo will contain. Is there a reason that even needs to be its own class?
[end bikeshedding]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done (moved the fields into the TelemetryInfo class)

// REQUIRED: Start time of an event
SteadyTimePoint start;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// REQUIRED: Start time of an event
SteadyTimePoint start;
/// Start time of an event
SteadyTimePoint start;

This says REQUIRED but there's a default constructor that doesn't initialize it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what caught Jonas's eye (though he probably doesn't know it) is that this class was very protobuf-y. Let me try to unpack that. We don't have the (understandable if you know the background, but strange if you don't) situation of fields that are optional at the transport layer but required at the application layer elsewhere. And I don't think we need that here either because the transport layer is part of the downstream/vendor implementation, so we can just use regular c++ conventions for the application layer (if a type is std::optional, it's optional; if it's not, it's not).

That's one part of the issue. The second part is question whether the "required-ness" of a field can be enforced syntactically (through a constructor which intializes it). That's usually a nice property though it doesn't work in all situations, and I don't think it's that useful for objects which are plain data carriers (no internal logic). It also only works if every constructor initializes it, so the fact that you've added a constructor which does that, but then also kept the default one which does not (well, it initializes it, but to zero), doesn't really help there.

I'm saying this because this overload set is basically the same as what you'd get if you specified no explicit constructors -- you could still do the same thing, just with curly braces (EventStats{} default initializes everything, EventStats{start} initializes the first member, EventStats{start, end} initializes both). So, if this is the behavior you want, you can just delete all of them. If you want to enforce the requiredness (which might be nice, but I don't think it's necessary -- Jonas may disagree), then you should delete the default constructor -- but then you also need to use these classes differently.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the confusion. Should ahve added more comment on this.
The "REQUIRED" here doesn't mean it has to be set at construction time .It just means it should be set at some point before the TelemetryInfo struct is sent off.

In any case, I've removed the EventStats and moved the start/end time fields directly into the TelemetryInfo struct.

// OPTIONAL: End time of an event - may be empty if not meaningful.
std::optional<SteadyTimePoint> end;
// TBD: could add some memory stats here too?

EventStats() = default;
EventStats(SteadyTimePoint start) : start(start) {}
EventStats(SteadyTimePoint start, SteadyTimePoint end)
: start(start), end(end) {}
};

/// Describes the exit signal of an event.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what the exit signal of an event means. Do you mean the exit signal of a process? Of LLDB itself?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's used by a few subclasses of the TelemetryInfo. (Comment below)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to go with Jonas here. "Signal" is a very confusing word here, because a process can die from a (unix) signal (but lldb doesn't do a very good job of differentiating the "death by signal N" and "exit with status N"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To answer the orignal question, this is used to describe both the exit of LLDB and a process (or main executable).
I've removed the "signal" from the comment.

struct ExitDescription {
int exit_code;
std::string description;
};

struct LldbBaseTelemetryInfo : public llvm::telemetry::TelemetryInfo {
EventStats stats;

std::optional<ExitDescription> exit_desc;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kinda surprised to see this here. I wouldn't expect this to make sense on every (or even most) of the telemetry entries.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're going to have 5 types of entries.I think the exit status (or more accurately "return status" for the command-info case) is applicable to the first 4 entries:

  • debugger-info (whether LLDB terminates successfully)
  • target-info (whether the main executable exits successfully)
  • command-info (whether the command executed successfully)
  • client-request-info (whether the request completed successfully)
  • misc-info (optionall)

So that's 4 out of 5, which is why I thought it made sense to hoist it into the common class.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this answers my previous question. It seems like the meaning of the exit code is sufficiently different (debugger and target have a process exit code, which is meaningful based on the OS) but commands and client request infos share nothing with that. It seems like separate enums would be appropriate for the latter. In other words, I think debugger-info and target-info each can have an exit code field (or an ExitDescription) member. The other 3 should have their own dedicated enum.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok -ll remove the field from LLDBBaseTelemetry. (But will leave the ExitDescription struct decl since the two of following PRs will use it)
SG?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be better to remove the struct completely for now. It'll be easier to figure out the exact meaning/description for it in the context of the PR which uses it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done - removed the ExistDescription


Debugger *debugger;

// For dyn_cast, isa, etc operations.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everywhere else we use /// LLVM RTTI support.

llvm::telemetry::KindType getKind() const override {
return LldbEntryKind::BaseInfo;
}

static bool classof(const llvm::telemetry::TelemetryInfo *t) {
// Subclasses of this is also acceptable.
return (t->getKind() & LldbEntryKind::BaseInfo) == LldbEntryKind::BaseInfo;
}

void serialize(llvm::telemetry::Serializer &serializer) const override;
};

/// The base Telemetry manager instance in LLDB
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: missing period.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

/// This class declares additional instrumentation points
/// applicable to LLDB.
class TelemetryManager : public llvm::telemetry::Manager {
public:
TelemetryManager(std::unique_ptr<llvm::telemetry::Config> config);

llvm::Error preDispatch(llvm::telemetry::TelemetryInfo *entry) override;

private:
std::unique_ptr<llvm::telemetry::Config> m_config;
};

} // namespace lldb_private
#endif // LLDB_CORE_TELEMETRY_H
4 changes: 2 additions & 2 deletions lldb/include/lldb/lldb-enumerations.h
Original file line number Diff line number Diff line change
Expand Up @@ -257,8 +257,8 @@ enum StopReason {
};

/// Command Return Status Types.
enum ReturnStatus {
eReturnStatusInvalid,
enum ReturnStatus : int {
eReturnStatusInvalid = 0,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

carry-over comment from previous patch("Why is this needed?")
This is needed to serialize the ReturnStatus object

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand that. What exactly happens if you leave this out?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when i tried to do json::Object({"ret_stat", returnStatus}), I got some build error (ambiguous types or sth like that).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, I'd suggest working around that locally (by casting the enum to an integer type?). It's kinda weird that this is the only enum which has an explicit type specifier.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done - removed the :int and =0

eReturnStatusSuccessFinishNoResult,
eReturnStatusSuccessFinishResult,
eReturnStatusSuccessContinuingNoResult,
Expand Down
13 changes: 12 additions & 1 deletion lldb/source/Core/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,15 @@ if (LLDB_ENABLE_CURSES)
endif()

# TODO: Add property `NO_PLUGIN_DEPENDENCIES` to lldbCore
set(LLDB_CORE_SRCS

)

if (LLVM_BUILD_TELEMETRY)
set(TELEMETRY_SOURCES Telemetry.cpp)
set(TELEMETRY_DEPS Telemetry)
endif()

add_lldb_library(lldbCore
Address.cpp
AddressRange.cpp
Expand Down Expand Up @@ -55,7 +64,8 @@ add_lldb_library(lldbCore
ThreadedCommunication.cpp
UserSettingsController.cpp
Value.cpp

${TELEMETRY_SOURCES}
PARTIAL_SOURCES_INTENDED
DEPENDS
clang-tablegen-targets

Expand All @@ -80,6 +90,7 @@ add_lldb_library(lldbCore
Support
Demangle
TargetParser
${TELEMETRY_DEPS}
)

add_dependencies(lldbCore
Expand Down
81 changes: 81 additions & 0 deletions lldb/source/Core/Telemetry.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
//===-- Telemetry.cpp -----------------------------------------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
#include "lldb/Core/Telemetry.h"
#include "lldb/Core/Debugger.h"
#include "lldb/Target/Statistics.h"
#include "lldb/Utility/ConstString.h"
#include "lldb/Utility/LLDBLog.h"
#include "lldb/Utility/UUID.h"
#include "lldb/Version/Version.h"
#include "lldb/lldb-enumerations.h"
#include "lldb/lldb-forward.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/Twine.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/RandomNumberGenerator.h"
#include "llvm/Telemetry/Telemetry.h"
#include <chrono>
#include <cstdlib>
#include <ctime>
#include <memory>
#include <string>
#include <utility>
#include <vector>

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete empty line

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

namespace lldb_private {

using ::llvm::Error;
using ::llvm::telemetry::Destination;
using ::llvm::telemetry::Serializer;
using ::llvm::telemetry::TelemetryInfo;

static uint64_t ToNanosec(const SteadyTimePoint Point) {
return std::chrono::nanoseconds(Point.time_since_epoch()).count();
}

void LldbBaseTelemetryInfo::serialize(Serializer &serializer) const {
serializer.write("entry_kind", getKind());
serializer.write("session_id", SessionId);
serializer.write("start_time", ToNanosec(stats.start));
if (stats.end.has_value())
serializer.write("end_time", ToNanosec(stats.end.value()));
if (exit_desc.has_value()) {
serializer.write("exit_code", exit_desc->exit_code);
serializer.write("exit_msg", exit_desc->description);
}
}

static std::string MakeUUID(lldb_private::Debugger *debugger) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/lldb_private:://, goes for the whole file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(all addressed in #126757)

std::string ret;
uint8_t random_bytes[16];
if (auto ec = llvm::getRandomBytes(random_bytes, 16)) {
LLDB_LOG(GetLog(LLDBLog::Object),
"Failed to generate random bytes for UUID: {0}", ec.message());
// fallback to using timestamp + debugger ID.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/fallback/Fallback/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

ret = llvm::formatv(
"{0}_{1}", std::chrono::steady_clock::now().time_since_epoch().count(),
debugger->GetID());
} else {
ret = lldb_private::UUID(random_bytes).GetAsString();
}

return ret;
}

TelemetryManager::TelemetryManager(
std::unique_ptr<llvm::telemetry::Config> config)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're using using ::llvm::telemetry::Foo for some other things but not for the Config. Why not use using namespace llvm::telemetry instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

: m_config(std::move(config)) {}

llvm::Error TelemetryManager::preDispatch(TelemetryInfo *entry) {
// Do nothing for now.
// In up-coming patch, this would be where the manager
// attach the session_uuid to the entry.
}

} // namespace lldb_private