[libcamera-devel] libcamera: log: add colors to log levels
diff mbox series

Message ID 20210203181746.22028-1-m.cichy@pengutronix.de
State Changes Requested
Headers show
Series
  • [libcamera-devel] libcamera: log: add colors to log levels
Related show

Commit Message

Marian Cichy Feb. 3, 2021, 6:17 p.m. UTC 
mono-colored wall of logs can be hard to read and doesn't show the level
of failure at a first glance. By adding colors to the log level
categories, it is much easier to scroll through logs and find important
entries.
---
 src/libcamera/log.cpp | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Sebastian Fricke Feb. 3, 2021, 7:09 p.m. UTC | #1 
Hey Marian,

Thank you for the patch.
This looks better than before, I tested it on my machine and it worked
without problems.
If you like to you can add:
Tested-by: Sebastian Fricke <sebastian.fricke@posteo.net>

Greetings,
Sebastian

On 03.02.2021 19:17, Marian Cichy wrote:
>mono-colored wall of logs can be hard to read and doesn't show the level
>of failure at a first glance. By adding colors to the log level
>categories, it is much easier to scroll through logs and find important
>entries.
>---
> src/libcamera/log.cpp | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
>diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
>index 45c7c2d2..3ad9c3de 100644
>--- a/src/libcamera/log.cpp
>+++ b/src/libcamera/log.cpp
>@@ -85,11 +85,11 @@ static int log_severity_to_syslog(LogSeverity severity)
> static const char *log_severity_name(LogSeverity severity)
> {
> 	static const char *const names[] = {
>-		"DEBUG",
>-		" INFO",
>-		" WARN",
>-		"ERROR",
>-		"FATAL",
>+		"\033[1m\033[37mDEBUG\033[0m",
>+		"\033[1m\033[32m INFO\033[0m",
>+		"\033[1m\033[33m WARN\033[0m",
>+		"\033[1m\033[31mERROR\033[0m",
>+		"\033[1m\033[35mFATAL\033[0m",
> 	};
>
> 	if (static_cast<unsigned int>(severity) < std::size(names))
>-- 
>2.20.1
>
>_______________________________________________
>libcamera-devel mailing list
>libcamera-devel@lists.libcamera.org
>https://lists.libcamera.org/listinfo/libcamera-devel
Marian Cichy Feb. 3, 2021, 7:13 p.m. UTC | #2 
Hi Sebastian,

thanks, I also just realized that I probably should sign-off, right?

should I send a V2?

regards,

marian

On 2/3/21 8:09 PM, Sebastian Fricke wrote:
> Hey Marian,
>
> Thank you for the patch.
> This looks better than before, I tested it on my machine and it worked
> without problems.
> If you like to you can add:
> Tested-by: Sebastian Fricke <sebastian.fricke@posteo.net>
>
> Greetings,
> Sebastian
>
> On 03.02.2021 19:17, Marian Cichy wrote:
>> mono-colored wall of logs can be hard to read and doesn't show the level
>> of failure at a first glance. By adding colors to the log level
>> categories, it is much easier to scroll through logs and find important
>> entries.
>> ---
>> src/libcamera/log.cpp | 10 +++++-----
>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
>> index 45c7c2d2..3ad9c3de 100644
>> --- a/src/libcamera/log.cpp
>> +++ b/src/libcamera/log.cpp
>> @@ -85,11 +85,11 @@ static int log_severity_to_syslog(LogSeverity 
>> severity)
>> static const char *log_severity_name(LogSeverity severity)
>> {
>>     static const char *const names[] = {
>> -        "DEBUG",
>> -        " INFO",
>> -        " WARN",
>> -        "ERROR",
>> -        "FATAL",
>> +        "\033[1m\033[37mDEBUG\033[0m",
>> +        "\033[1m\033[32m INFO\033[0m",
>> +        "\033[1m\033[33m WARN\033[0m",
>> +        "\033[1m\033[31mERROR\033[0m",
>> +        "\033[1m\033[35mFATAL\033[0m",
>>     };
>>
>>     if (static_cast<unsigned int>(severity) < std::size(names))
>> -- 
>> 2.20.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
Jean-Michel Hautbois Feb. 3, 2021, 7:26 p.m. UTC | #3 
Hi Marien,

Thanks for the patch !

On 03/02/2021 20:13, Marian Cichy wrote:
> Hi Sebastian,
> 
> thanks, I also just realized that I probably should sign-off, right?
> 
> should I send a V2?
> 
> regards,
> 
> marian
> 
> On 2/3/21 8:09 PM, Sebastian Fricke wrote:
>> Hey Marian,
>>
>> Thank you for the patch.
>> This looks better than before, I tested it on my machine and it worked
>> without problems.
>> If you like to you can add:
>> Tested-by: Sebastian Fricke <sebastian.fricke@posteo.net>
>>
>> Greetings,
>> Sebastian
>>
>> On 03.02.2021 19:17, Marian Cichy wrote:
>>> mono-colored wall of logs can be hard to read and doesn't show the level
>>> of failure at a first glance. By adding colors to the log level
>>> categories, it is much easier to scroll through logs and find important
>>> entries.
>>> ---
>>> src/libcamera/log.cpp | 10 +++++-----
>>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
>>> index 45c7c2d2..3ad9c3de 100644
>>> --- a/src/libcamera/log.cpp
>>> +++ b/src/libcamera/log.cpp
>>> @@ -85,11 +85,11 @@ static int log_severity_to_syslog(LogSeverity
>>> severity)
>>> static const char *log_severity_name(LogSeverity severity)
>>> {
>>>     static const char *const names[] = {
>>> -        "DEBUG",
>>> -        " INFO",
>>> -        " WARN",
>>> -        "ERROR",
>>> -        "FATAL",
>>> +        "\033[1m\033[37mDEBUG\033[0m",
>>> +        "\033[1m\033[32m INFO\033[0m",
>>> +        "\033[1m\033[33m WARN\033[0m",
>>> +        "\033[1m\033[31mERROR\033[0m",
>>> +        "\033[1m\033[35mFATAL\033[0m",
>>>     };

I like the idea, but I would prefer having the possibility to
enable/disable it at runtime, as for copy/pasting debug logs it is
easier to read uncolored :-).

JM
Sebastian Fricke Feb. 3, 2021, 7:30 p.m. UTC | #4 
On 03.02.2021 20:13, Marian Cichy wrote:
>Hi Sebastian,
>
>thanks, I also just realized that I probably should sign-off, right?
>
>should I send a V2?

I would probably not send it right away and wait for some feedback from
the others first.

Greetings,
Sebastian

>
>regards,
>
>marian
>
>On 2/3/21 8:09 PM, Sebastian Fricke wrote:
>>Hey Marian,
>>
>>Thank you for the patch.
>>This looks better than before, I tested it on my machine and it worked
>>without problems.
>>If you like to you can add:
>>Tested-by: Sebastian Fricke <sebastian.fricke@posteo.net>
>>
>>Greetings,
>>Sebastian
>>
>>On 03.02.2021 19:17, Marian Cichy wrote:
>>>mono-colored wall of logs can be hard to read and doesn't show the level
>>>of failure at a first glance. By adding colors to the log level
>>>categories, it is much easier to scroll through logs and find important
>>>entries.
>>>---
>>>src/libcamera/log.cpp | 10 +++++-----
>>>1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>>diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
>>>index 45c7c2d2..3ad9c3de 100644
>>>--- a/src/libcamera/log.cpp
>>>+++ b/src/libcamera/log.cpp
>>>@@ -85,11 +85,11 @@ static int log_severity_to_syslog(LogSeverity 
>>>severity)
>>>static const char *log_severity_name(LogSeverity severity)
>>>{
>>>    static const char *const names[] = {
>>>-        "DEBUG",
>>>-        " INFO",
>>>-        " WARN",
>>>-        "ERROR",
>>>-        "FATAL",
>>>+        "\033[1m\033[37mDEBUG\033[0m",
>>>+        "\033[1m\033[32m INFO\033[0m",
>>>+        "\033[1m\033[33m WARN\033[0m",
>>>+        "\033[1m\033[31mERROR\033[0m",
>>>+        "\033[1m\033[35mFATAL\033[0m",
>>>    };
>>>
>>>    if (static_cast<unsigned int>(severity) < std::size(names))
>>>-- 
>>>2.20.1
>>>
>>>_______________________________________________
>>>libcamera-devel mailing list
>>>libcamera-devel@lists.libcamera.org
>>>https://lists.libcamera.org/listinfo/libcamera-devel
>>_______________________________________________
>>libcamera-devel mailing list
>>libcamera-devel@lists.libcamera.org
>>https://lists.libcamera.org/listinfo/libcamera-devel
>>
Andrey Konovalov Feb. 3, 2021, 7:58 p.m. UTC | #5 
Hi,

On 03.02.2021 22:26, Jean-Michel Hautbois wrote:
> Hi Marien,
> 
> Thanks for the patch !
> 
> On 03/02/2021 20:13, Marian Cichy wrote:
>> Hi Sebastian,
>>
>> thanks, I also just realized that I probably should sign-off, right?
>>
>> should I send a V2?
>>
>> regards,
>>
>> marian
>>
>> On 2/3/21 8:09 PM, Sebastian Fricke wrote:
>>> Hey Marian,
>>>
>>> Thank you for the patch.
>>> This looks better than before, I tested it on my machine and it worked
>>> without problems.
>>> If you like to you can add:
>>> Tested-by: Sebastian Fricke <sebastian.fricke@posteo.net>
>>>
>>> Greetings,
>>> Sebastian
>>>
>>> On 03.02.2021 19:17, Marian Cichy wrote:
>>>> mono-colored wall of logs can be hard to read and doesn't show the level
>>>> of failure at a first glance. By adding colors to the log level
>>>> categories, it is much easier to scroll through logs and find important
>>>> entries.
>>>> ---
>>>> src/libcamera/log.cpp | 10 +++++-----
>>>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
>>>> index 45c7c2d2..3ad9c3de 100644
>>>> --- a/src/libcamera/log.cpp
>>>> +++ b/src/libcamera/log.cpp
>>>> @@ -85,11 +85,11 @@ static int log_severity_to_syslog(LogSeverity
>>>> severity)
>>>> static const char *log_severity_name(LogSeverity severity)
>>>> {
>>>>      static const char *const names[] = {
>>>> -        "DEBUG",
>>>> -        " INFO",
>>>> -        " WARN",
>>>> -        "ERROR",
>>>> -        "FATAL",
>>>> +        "\033[1m\033[37mDEBUG\033[0m",
>>>> +        "\033[1m\033[32m INFO\033[0m",
>>>> +        "\033[1m\033[33m WARN\033[0m",
>>>> +        "\033[1m\033[31mERROR\033[0m",
>>>> +        "\033[1m\033[35mFATAL\033[0m",
>>>>      };
> 
> I like the idea, but I would prefer having the possibility to
> enable/disable it at runtime, as for copy/pasting debug logs it is
> easier to read uncolored :-).

I second this proposal!
Maybe something similar to GST_DEBUG_NO_COLOR in gstreamer.

Thanks,
Andrey

> JM
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
Laurent Pinchart Feb. 3, 2021, 10:07 p.m. UTC | #6 
Hi Marian,

Thank you for the patch.

On Wed, Feb 03, 2021 at 10:58:42PM +0300, Andrey Konovalov wrote:
> On 03.02.2021 22:26, Jean-Michel Hautbois wrote:
> > On 03/02/2021 20:13, Marian Cichy wrote:
> >> Hi Sebastian,
> >>
> >> thanks, I also just realized that I probably should sign-off, right?
> >>
> >> should I send a V2?

That's right. A Signed-off-by line is required as explained in
Documentation/contributing.rst.

> >> On 2/3/21 8:09 PM, Sebastian Fricke wrote:
> >>> Hey Marian,
> >>>
> >>> Thank you for the patch.
> >>> This looks better than before, I tested it on my machine and it worked
> >>> without problems.
> >>> If you like to you can add:
> >>> Tested-by: Sebastian Fricke <sebastian.fricke@posteo.net>
> >>>
> >>> Greetings,
> >>> Sebastian
> >>>
> >>> On 03.02.2021 19:17, Marian Cichy wrote:
> >>>> mono-colored wall of logs can be hard to read and doesn't show the level
> >>>> of failure at a first glance. By adding colors to the log level
> >>>> categories, it is much easier to scroll through logs and find important
> >>>> entries.
> >>>> ---
> >>>> src/libcamera/log.cpp | 10 +++++-----
> >>>> 1 file changed, 5 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
> >>>> index 45c7c2d2..3ad9c3de 100644
> >>>> --- a/src/libcamera/log.cpp
> >>>> +++ b/src/libcamera/log.cpp
> >>>> @@ -85,11 +85,11 @@ static int log_severity_to_syslog(LogSeverity
> >>>> severity)
> >>>> static const char *log_severity_name(LogSeverity severity)
> >>>> {
> >>>>      static const char *const names[] = {
> >>>> -        "DEBUG",
> >>>> -        " INFO",
> >>>> -        " WARN",
> >>>> -        "ERROR",
> >>>> -        "FATAL",
> >>>> +        "\033[1m\033[37mDEBUG\033[0m",
> >>>> +        "\033[1m\033[32m INFO\033[0m",
> >>>> +        "\033[1m\033[33m WARN\033[0m",
> >>>> +        "\033[1m\033[31mERROR\033[0m",
> >>>> +        "\033[1m\033[35mFATAL\033[0m",
> >>>>      };
> > 
> > I like the idea, but I would prefer having the possibility to
> > enable/disable it at runtime, as for copy/pasting debug logs it is
> > easier to read uncolored :-).
> 
> I second this proposal!
> Maybe something similar to GST_DEBUG_NO_COLOR in gstreamer.

This sounds good to me, but I think we should go one step further and
disable colours by default when the log target isn't a terminal.
libcamera supports 3 log targets so far:

- LoggingTargetSyslog
- LoggingTargetFile
- LoggingTargetStream

When logging to syslog or to a file, I think we should disable colours
by default. When logging to a stream, we should check if the stream
refers to a terminal, and disable colours if it doesn't. The question is
how to do so. The isatty() function is a good start, but it takes a file
descriptor, and as far as I can tell, there's no way to get a file
descriptor from an std::ostream.

Patch
diff mbox series 

diff --git a/src/libcamera/log.cpp b/src/libcamera/log.cpp
index 45c7c2d2..3ad9c3de 100644
--- a/src/libcamera/log.cpp
+++ b/src/libcamera/log.cpp
@@ -85,11 +85,11 @@  static int log_severity_to_syslog(LogSeverity severity)
 static const char *log_severity_name(LogSeverity severity)
 {
 	static const char *const names[] = {
-		"DEBUG",
-		" INFO",
-		" WARN",
-		"ERROR",
-		"FATAL",
+		"\033[1m\033[37mDEBUG\033[0m",
+		"\033[1m\033[32m INFO\033[0m",
+		"\033[1m\033[33m WARN\033[0m",
+		"\033[1m\033[31mERROR\033[0m",
+		"\033[1m\033[35mFATAL\033[0m",
 	};
 
 	if (static_cast<unsigned int>(severity) < std::size(names))