Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document logging configuration #3397

Merged
merged 13 commits into from
Mar 14, 2023
Merged

Document logging configuration #3397

merged 13 commits into from
Mar 14, 2023

Conversation

andreyaksenov
Copy link
Contributor

@andreyaksenov andreyaksenov commented Mar 9, 2023

Documented how to configure logging in the log module and improved a bit other logging docs along the way.
Here is the summary of the changes:

  • Moved the Feedback section outside of Logging as six-level deep nesting looks no good.
  • Updated the logging example to use a numbered list for better readability.
  • In the Module log topic, added a log.cfg description with an example and links to corresponding box.cfg properties.

Resolves #1974

@andreyaksenov andreyaksenov linked an issue Mar 9, 2023 that may be closed by this pull request
@andreyaksenov andreyaksenov marked this pull request as ready for review March 13, 2023 07:49
Copy link
Contributor

@p7nov p7nov left a comment

Choose a reason for hiding this comment

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

Some comments from my side

@@ -11,7 +15,7 @@
.. confval:: log_level

Since version 1.6.2.
What level of detail the :ref:`log <admin-logs>` will have. There are seven levels:
Specifies the level of detail the :ref:`log <admin-logs>` has. There are seven levels:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just drop the verb?
"The level of detail..."

Copy link
Contributor Author

@andreyaksenov andreyaksenov Mar 14, 2023

Choose a reason for hiding this comment

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

I'd keep Specifies here as it shows that the property value can be get and set. What do you think?

By setting log_level, one can enable logging of all classes below
or equal to the given level. Tarantool prints its logs to the standard
By setting ``log_level``, you can enable logging of all events with severities above
or equal to the given level. Tarantool prints logs to the standard
error stream by default, but this can be changed with the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
error stream by default, but this can be changed with the
error stream by default. This can be changed with the

@@ -42,36 +46,41 @@

Since version 1.7.4.
By default, Tarantool sends the log to the standard error stream
(``stderr``). If ``log`` is specified, Tarantool sends the log to a file,
or to a pipe, or to the system logger.
(``stderr``). If ``log`` is specified, Tarantool can send the log to a ...
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure that the list below improves the text significantly: all the options are just 1-2 words and don't have explanations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway, in this case there are punctuation issues:

  • ... instead of : before the list
  • ; in the end of list items (see our style guide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure that the list below improves the text significantly: all the options are just 1-2 words and don't have explanations.

Generally, I agree but still think it's better than three or in a row :)

@@ -32,7 +36,7 @@
| Dynamic: **yes**

Warning: prior to Tarantool 1.7.5 there were only six levels and ``DEBUG`` was
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe turn this into a real warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Converted to a note


Example setting for sending the log to a file:
The example below shows how to send logs to the ``tarantool.log`` file:
Copy link
Contributor

Choose a reason for hiding this comment

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

Idea: make examples less "narrative" but more structured:

Suggested change
The example below shows how to send logs to the ``tarantool.log`` file:
Example 1: sending the log to the ``tarantool.log`` file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I like it.

directory. If the ``log`` string has no prefix or has the prefix "file:",
then the string is interpreted as a file path.

Example setting for sending the log to a pipe:
This example shows how to send the log to a pipe:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This example shows how to send the log to a pipe:
Example 2: sending the log to a pipe.

If the ``log`` string begins with '|' or has the prefix "pipe:",
then the string is interpreted as a Unix
`pipeline <https://en.wikipedia.org/wiki/Pipeline_%28Unix%29>`_.

Example setting for sending the log to syslog:
The example below shows how to send the log to syslog:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The example below shows how to send the log to syslog:
Example 3: sending the log to syslog.

`syslogd <http://www.rfc-base.org/txt/rfc-5424.txt>`_ program which normally
is running in the background of any Unix-like platform.
`syslogd <https://linux.die.net/man/8/syslogd>`_ program, which normally
is running in the background on any Unix-like platform.
The setting can be 'syslog:', 'syslog:facility=...', 'syslog:identity=...',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The setting can be 'syslog:', 'syslog:facility=...', 'syslog:identity=...',
The setting can be ``syslog:``, ``syslog:facility=...``, ``syslog:identity=...``,


.. function:: log.cfg({})

Allows you to configure logging options.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe drop "Allows you to"?

Copy link
Contributor Author

@andreyaksenov andreyaksenov Mar 14, 2023

Choose a reason for hiding this comment

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

I'd keep it as is - I think, Allows you to... or Provides the ability to ... suits good for complex configuration options that don't do a specific thing but provide access to a bunch of related properties.


:return: nil

The actual output will be a line in the log, containing:

* the current timestamp,
* a module name,
* 'E', 'W', 'I', 'V' or 'D' depending on ``log_level_function_name``, and
* 'E', 'W', 'I', 'V' or 'D' depending on the called function, and
Copy link
Contributor

Choose a reason for hiding this comment

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

"and" seems awkward to me here

Copy link
Contributor Author

@andreyaksenov andreyaksenov Mar 14, 2023

Choose a reason for hiding this comment

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

agree, didn't catch it when reviewing old docs. AFAIU, commas aren't required here, according to the style guide.

Copy link
Contributor

@p7nov p7nov left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@andreyaksenov andreyaksenov merged commit e88488d into latest Mar 14, 2023
@andreyaksenov andreyaksenov deleted the logging branch March 14, 2023 09:59
p7nov pushed a commit that referenced this pull request Mar 24, 2023
* Document logging configuration
* Document default log_nonblock values for different loggers

Resolves #1974
Resolves #2242
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[2pt] feedback: Configuration reference [3pt] Logging configuration
3 participants