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

Add documentation for the log configuration #45940

Merged
merged 4 commits into from
Aug 30, 2024

Conversation

vapopov
Copy link
Contributor

@vapopov vapopov commented Aug 28, 2024

In this PR added documentation for the logger configuration in Teleport, since filesystem watcher added for the log file we should noted about new behaviour in case of move/rename/delete the log file. Also sample configuration for logrotate is added.

Related: #43359

Copy link

🤖 Vercel preview here: https://docs-l9f2tvxbw-goteleport.vercel.app/docs/ver/preview

Other available options for defining the output include `stdout` (aliases `out` or `1`), `syslog` for writing
to the syslog file, or a filepath for direct writing to a log file destination.

Severity has several levels, which are sorted by increasing priority (means that if `info` is selected, `warn` and `err` also applied):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Severity has several levels, which are sorted by increasing priority (means that if `info` is selected, `warn` and `err` also applied):
Severity has several levels, which are sorted by decreasing priority (means that if `info` is selected, `warn` and `err` also applied):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

slog misleads, they have different ordering with logrus

slog.LevelError = 8
slog.LevelWarn = 4

logrus.ErrorLevel = 2
logrus.WarnLevel = 3

Copy link

🤖 Vercel preview here: https://docs-cm3ka1jr8-goteleport.vercel.app/docs/ver/preview

Copy link
Contributor

@ptgott ptgott left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together!

I think it would make sense to move this to docs/pages/admin-guides/management/diagnostics, where we explain other Teleport signals such as profiles, metrics, and traces.

docs/pages/admin-guides/management/admin/logging.mdx Outdated Show resolved Hide resolved
docs/pages/admin-guides/management/admin/logging.mdx Outdated Show resolved Hide resolved
docs/pages/admin-guides/management/admin/logging.mdx Outdated Show resolved Hide resolved
docs/pages/admin-guides/management/admin/logging.mdx Outdated Show resolved Hide resolved
docs/pages/admin-guides/management/admin/logging.mdx Outdated Show resolved Hide resolved
Other available options for defining the output include `stdout` (aliases `out` or `1`), `syslog` for writing
to the syslog file, or a filepath for direct writing to a log file destination.

Severity has several levels, which are sorted by decreasing priority (means that if `info` is selected, `warn` and `err` also applied):
Copy link
Contributor

Choose a reason for hiding this comment

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

(means that if `info` is selected, `warn` and `err` also applied)

We should fix the grammar here and make this a complete sentence. I think it would make sense to separate this into its own sentence rather than use parentheses.

to the syslog file, or a filepath for direct writing to a log file destination.

Severity has several levels, which are sorted by decreasing priority (means that if `info` is selected, `warn` and `err` also applied):
- `err`, `error` - used for errors that should definitely be noted.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "should definitely be noted" mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Means that these log messages indicate actions are required to fix them, errors may lead to an unpredictable state of the application

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say, "Logs that require action from the user" instead of "Should definitely be noted".

Comment on lines 31 to 32
- `debug` - usually only enabled when debugging, verbose logging.
- `trace` - designates more detailed informational events than the debug.
Copy link
Contributor

Choose a reason for hiding this comment

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

The descriptions here are self evident, I think. Is there anything we can say about the kinds of logs a user can expect when enabling debug or trace-level logging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

trace usually shows very detail information like sequence of method executions
Did simple search by code base, so we don't use this level that much:

s.log.WithField("event", event).Trace("sessionCache: Received watcher event")
s.log.WithField("session_id", s.ID()).Trace("session will be recorded at proxy")
log.WithFields(log.Fields{
			"src_addr":     fmt.Sprintf("%v", source),
			"dst_addr":     fmt.Sprintf("%v", destination),
			"cluster_name": p.clusterName}).Trace("Successfully generated signed PROXY header")
// Okta plugin.
log.Trace("Collating plugin resource")
log.Trace("Installing plugin")

- `debug` - usually only enabled when debugging, verbose logging.
- `trace` - designates more detailed informational events than the debug.

The default format for log output is `text`. Another available format is `json`, which may simplify log
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "may simplify" mean here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

means that json already has a structure for parsing with field names, if we use text, usually regular expression is used to extract data from text message

Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

@vapopov You also need to update config.js to include the new page in the left navigation bar.

docs/pages/admin-guides/management/admin/logging.mdx Outdated Show resolved Hide resolved
docs/pages/admin-guides/management/admin/logging.mdx Outdated Show resolved Hide resolved
docs/pages/admin-guides/management/admin/logging.mdx Outdated Show resolved Hide resolved
docs/pages/admin-guides/management/admin/logging.mdx Outdated Show resolved Hide resolved
Copy link

🤖 Vercel preview here: https://docs-2zyouwui1-goteleport.vercel.app/docs/ver/preview

- `warn`, `warning` - non-critical entries that deserve attention.
- `info` or empty value - general operational entries about what's going on inside the application.
- `debug` - usually only enabled when debugging, verbose logging.
- `trace` - designates more detailed information about actions and events.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we ever use "trace" level in our code? I don't think we do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we use, but very limited #45940 (comment), so might be better to remove?

@vapopov
Copy link
Contributor Author

vapopov commented Aug 30, 2024

@ptgott, could you please take another look at the PR?

Copy link

🤖 Vercel preview here: https://docs-kxytd1emr-goteleport.vercel.app/docs/ver/preview

@vapopov vapopov force-pushed the vapopov/log-filesystem-watcher-doc branch from 38decd6 to aa5ac4b Compare August 30, 2024 21:18
Copy link

🤖 Vercel preview here: https://docs-m1lb76i8a-goteleport.vercel.app/docs/ver/preview

@vapopov vapopov added this pull request to the merge queue Aug 30, 2024
Merged via the queue into master with commit c018568 Aug 30, 2024
40 checks passed
@vapopov vapopov deleted the vapopov/log-filesystem-watcher-doc branch August 30, 2024 21:40
@public-teleport-github-review-bot

@vapopov See the table below for backport results.

Branch Result
branch/v16 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v16 documentation no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants