[Lldb-commits] [PATCH] D107079: [lldb] Add an option for emitting LLDB logs as JSON

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

I have a few little grammatical nits.

The suggestion to use EnumArgs rather than a fresh argument type you can do or not as you see fit.

It really seems to me that SBDebugger.EnableLog should provide a way to set the OutputFormat.  For humans, the plain text is probably preferable, but for programs that run lldb and want to control logging output, JSON would seem more natural.  And programs that run lldb are more likely to want to use EnableLog than HandleCommand, since we've provided it...

But if you do that the OutputFormat should be a public enum, it would be dopey to have an API that takes a char * of "plain" or "json".

There should be a setting for the preferred OutputFormat.



================
Comment at: lldb/docs/design/logging.rst:23
+
+LLDB allows configuring format that log messages are emitted in and what
+metainformation should be attached to each log message. These options can
----------------
You at least need "the format", but maybe

configuring the format with which log messages are emitted and the metainformation which should...

is a little easier to parse.


================
Comment at: lldb/docs/design/logging.rst:25
+metainformation should be attached to each log message. These options can
+be set for each logging channel (but not separately for each logging category).
+The output of each channel can also be directed to write to a log file,
----------------
These options are applied to log channels, and apply to all enabled categories in that channel.





================
Comment at: lldb/docs/design/logging.rst:47
+
+The current JSON log format puts each log message in its own JSON object. Each
+object also contains a trailing comma character (``,``). The intended way of
----------------
This makes it not proper JSON by itself, right?  I'm guessing you do it this way so you don't have to track and last log emission?  I can see that that would be more trouble than it's worth, so I'm not objecting.  It is a little weird to call this JSON format when it is just JSON-adjacent, but that's probably too picky...


================
Comment at: lldb/docs/design/logging.rst:65
+set.
+
+.. list-table:: JSON message fields
----------------
We always know which channel/category pair produces the message.  I think it would be handy to also always include the channel/category pairs.  That seems like it would be useful information.


================
Comment at: lldb/docs/design/logging.rst:76
+     - The log message itself.
+   * - ``sequence-id``
+     - Number
----------------
The sequence ID is useful when the output is unstructured, but do we really need it in the structured form?  This is just the index of the message in the JSON vector.


================
Comment at: lldb/docs/design/logging.rst:82
+     - The number of seconds since the host systems epoch (usually UNIX time).
+   * - ``pid``
+     - Number
----------------
Seems like the pid & tid are always ganged together.  Maybe indicate that here?

But it does seem a shame that to get the thread info, I have to pay for "pid = NNNN" in every output message.

Seems like the pid should go in some sort of header, it won't change.  You didn't add this behavior, but putting it in the JSON makes the pid emission cost about twice what it does in text form.


================
Comment at: lldb/include/lldb/Utility/Log.h:60
+  /// The format used when emitting log messages.
+  enum class OutputFormat {
+    /// Log messages are printed as plain text.
----------------
I was a little surprised to see this enum in Log.h and not in the lldb public enums.  Don't you need that so you can make an SB API to set the log output format?  And don't you need that API?


================
Comment at: lldb/source/Commands/Options.td:446
     Desc<"Prepend the names of files and function that generate the logs.">;
+  def log_format : Option<"format", "O">, Group<1>, Arg<"LogFormat">,
+    Desc<"Specify the used log format for the enabled channels.">;
----------------
You could also do this with the EnumArg descriptor.  That way the help will show the options.  I'm pretty sure that also auto-gen's the completion for the option as well. 


================
Comment at: lldb/unittests/Utility/LogTest.cpp:275
+
+TEST_F(LogChannelEnabledTest, PlainTextFormat) {
+  std::string error;
----------------
Since you allow the Output format to be changed while the channel stays enabled, you should add a test for that.  You code should handle that gracefully, from what I can see.  But still...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107079/new/

https://reviews.llvm.org/D107079