[Lldb-commits] [lldb] [lldb][telemetry] Implement LLDB Telemetry (part 1) (PR #119716)

[Pavel Labath via lldb-commits]

================
@@ -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 {
+  // REQUIRED: Start time of an event
+  SteadyTimePoint start;
----------------
labath wrote:

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.

https://github.com/llvm/llvm-project/pull/119716