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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think that a structured logging format could be very useful. I am not sure what to make of the trailing "," in the json format though. It seems its usefulness is fairly limited, given that additional post processing is still needed to turn it into valid json. One could envision printing the surrounding `[`/`]` when logging is enabled/disabled, but I'm not sure that's such a good idea since one often inspects logs from crashing lldb instances. I have a feeling it would be cleanest to just drop the comma entirely. That way at least each individual log message would be valid json (so one could feed it to some log analyzer/storage backend in real time), and adding the trailing comma would would still be fairly trivial in post processing.

I too think that we should keep the textual log format, because it's much easier to read when printing to the terminal interactively. One thing to note there is that this patch removes the column alignment code from the textual output. With the advent of the structured format, this may not be so useful, as uninteresting fields can be removed differently (right now, I use vi block editing commands for that), but some alignment might still be useful for the ease of reading.

In D107079#2939328 <https://reviews.llvm.org/D107079#2939328>, @teemperor wrote:

> - Added the log format as an option to `log enable` (`o`, `f` and `F` are taken, so I just picked `O` as the short option).

This seems like a good time to remind everyone that not every option needs to have a short form. I think that skipping one is a better idea than picking a random letter that noone is going to remember.

In D107079#2913864 <https://reviews.llvm.org/D107079#2913864>, @mib wrote:

> This looks like a great effort to improve logging and debugging! Have you considered also adding logging levels (info, debug, warning, error ...) ? I think the `CommandReturnObject` and `Status` classes already makes the distinction between warnings and errors messages. We could enrich logging further more by having logging levels to filter / color-code the messages.

We had a concept of different log levels at one point, but it wasn't really used, so it was removed during some refactoring. Though, with a structured log format, I can see how it could be useful to reintroduce that.

In D107079#2914111 <https://reviews.llvm.org/D107079#2914111>, @aprantl wrote:

> More a comment than anything else: One thing I always wanted to explore was to implement LLDB's logging on Darwin on top of os_log (https://developer.apple.com/documentation/os/logging) which is also structured and also faster since it moves the the formatting out of process. This patch is great because it also works for non-Darwin platforms, and I guess there isn't anything preventing us from moving to os_log even if we take patch.

BTW, there's a function called Host::SystemLog, which (I think) logs to some darwin-specific facility on darwin hosts. Personally, I think that having two logging mechanisms is just confusing, but I think I would be interesting if you could redirect the "normal" lldb logs to some os-specific target (like you can do with a file).



================
Comment at: lldb/docs/design/logging.rst:65
+set.
+
+.. list-table:: JSON message fields
----------------
jingham wrote:
> 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.
The way logging works right now, we only know which channel the message came from. Category is lost, and sometimes it's never really clear because of stuff like GetLogIfAny/AllCategoriesSet(CAT1 | CAT2).

To be able to do this (which I think is a great idea), we'd need to slightly refactor the logging code, so thateach message can be uniquely associated with a specific category.


================
Comment at: lldb/source/Utility/Log.cpp:314-320
+  void PrintSeparatorIfNeeded() {
+    // The first output at the start of the line doesn't need a separating
+    // space.
+    if (!m_anything_written)
+      return;
+    m_output << " ";
+  }
----------------
I guess you should use llvm::ListSeparator for this.


================
Comment at: lldb/source/Utility/Log.cpp:378-380
     llvm::SmallString<12> format_str;
     llvm::raw_svector_ostream format_os(format_str);
     format_os << "{0,-" << llvm::alignTo<16>(thread_name.size()) << "} ";
----------------
This is now dead code.


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

https://reviews.llvm.org/D107079