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

draft logging updates #10273

Merged
merged 16 commits into from
May 12, 2021
Merged

draft logging updates #10273

merged 16 commits into from
May 12, 2021

Conversation

taroface
Copy link
Contributor

@taroface taroface commented Apr 6, 2021

This PR (WIP):

  • Documents new features in the v21.1 logging system (under Deploy -> Logging)
  • Provides logging usage examples
  • Updates and consolidates previous logging documentation, which is now centered around Logging docs
  • Adds pages for auto-generated reference docs (under Reference -> Logging System)
  • Updates default logging behaviors for cockroach commands

Still in-progress:

  • Example of parsing network logs with Elasticsearch (cut from scope for this docs update. we have the technical network logging details documented already)
  • Reference docs aren't yet presentable, due to some formatting elements. This is pending approval of some structure and formatting updates in docs, server: update auto-generated logging docs cockroach#62629 (I also plan to make a further commit on that PR, now that I've previewed this on our site)
  • STORAGE is not represented in the logging usage examples. These are primarily used by CRL engineers for troubleshooting.
  • Need to add an example of JSON structured format + some differences from unstructured format (I think this can happen with the network parsing tutorial)

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@taroface
Copy link
Contributor Author

taroface commented Apr 7, 2021

@knz This PR still has some outstanding elements (see list above), but I wonder if you'd like to begin reviewing what is here. If you would prefer to wait, I can update you when all of the above are resolved.

cc @thtruo

@taroface taroface requested a review from thtruo April 8, 2021 21:30
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

This was a very pleasant read — I really like the use cases and the organization you've come up with.
Only few comments remaining.

Reviewed 37 of 37 files at r1, 4 of 4 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @taroface and @thtruo)


_includes/v21.1/misc/logging-flags.md, line 3 at r2 (raw file):

All of the below settings are defined in the YAML payload.

"All the below settings can now be defined in the YAML payload instead."


v21.1/cockroach-gen.md, line 118 at r2 (raw file):

### Logging

By default, the `gen` command logs messages to `stderr`. This includes events with `WARNING` [severity](logging.html#logging-levels) and higher.

Here and throughout: What about an include?

"By default, this command logs messages to...;

if you need to troubleshoot..."


v21.1/configure-logs.md, line 9 at r2 (raw file):

This page describes how to configure CockroachDB logs with the [`--log` flag](cockroach-start.html#logging) and a [YAML payload](#yaml-payload). Most logging behaviors are configurable, including:

- The [logging sinks](#configure-logging-sinks) that output logs to different locations, including over the network

nit: period at the end of each bullet point?


v21.1/configure-logs.md, line 68 at r2 (raw file):

## Configure logging sinks

A logging *sink* outputs log messages from specified [logging channels](#logging-overview.html#logging-channels).

"A logging sink represents one logical funnel through which the events from one or more logging channel are emitted externally to the cockroach process. Logging sinks are the unit of configurability for mapping events from inside to outside of CockroachDB."


v21.1/configure-logs.md, line 70 at r2 (raw file):

A logging *sink* outputs log messages from specified [logging channels](#logging-overview.html#logging-channels).

CockroachDB can write logs to [files](#output-to-files), a [Fluentd](https://www.fluentd.org/)-compatible [server](#output-to-network), and the [standard error stream (`stderr`)](#output-to-stderr). Each output is configured under `sinks`:

here and below: it would be useful to convey the idea that the list of sink types will likely grow over time. Currently the phrasing suggests that there are only 3 types and that choice is closed forever.


v21.1/configure-logs.md, line 79 at r2 (raw file):

      ...
  fluent-servers:
  	{server name}:

nit: stray tab character


v21.1/configure-logs.md, line 89 at r2 (raw file):

<a name="common-sink-parameters">

All three sink types use the following common parameters.

"All possible sink types .." (this will remain true even if we add more sink types)


v21.1/configure-logs.md, line 115 at r2 (raw file):

sinks:
  file-groups:
  	default:

nit: stray tab character, here and below.


v21.1/configure-logs.md, line 123 at r2 (raw file):

{{site.data.alerts.callout_success}}
A file group name is arbitrary and is used to name the log file. The `default` file group is an exception. For details, see [Log file naming](#log-file-naming).

name the log files


v21.1/configure-logs.md, line 159 at r2 (raw file):

{{site.data.alerts.callout_info}}
Channels in a file group named `default` are output to a `cockroach.log` file.

"The files generated for a group named default are named after the patterncockroach.XXXX.log"

(no need to mention the "channel" concept in this explanation)


v21.1/configure-logs.md, line 208 at r2 (raw file):

## Configure logging defaults

When setting up a logging configuration, it's simplest to define shared parameters in `file-defaults` and `fluent-defaults` and override specific values as needed in [`file-groups`](#output-to-files), [`fluent-servers`](#output-to-network), and [`stderr`](#output-to-stderr). For a complete example, see the [default configuration](#default-logging-configuration).

nit: here and below; do we conventionally use contractions? if not, "it's" -> "it is"


v21.1/configure-logs.md, line 221 at r2 (raw file):

~~~
/cockroachdb/cockroach/cockroach-data/logs

What is this example coming from? I don't see the path cockroachdb/cockroach being mentioned elsewhere.


v21.1/configure-logs.md, line 225 at r2 (raw file):

in its own directory

"in the directory pointed to by its logging configuration".

(it's possible, albeit not recommended, to point multiple crdb processes to the same directory)


v21.1/configure-logs.md, line 248 at r2 (raw file):

    dev:
      channels: [DEV]
	  dir: /custom/dir/path/

nit: stray tab characters


v21.1/configure-logs.md, line 262 at r2 (raw file):

The default message format for log files is [`crdb-v2`](logformats.html#format-crdb-v2). Each `crdb-v2` log message starts with a flat prefix that contains event metadata (e.g., severity, date, timestamp, channel), followed by the event payload. For details on the metadata, see [Log formats](logformats.html#format-crdb-v2).

If you plan to read logs programmatically, you can switch to JSON:

there are multiple JSON formats; how do you plan to explain that?


v21.1/configure-logs.md, line 306 at r2 (raw file):

- File and network sinks output messages with `INFO` severity or higher. Since `INFO` is the lowest severity level, this includes all log messages.
- `stderr` is configured with `filter: NONE` and does not receive log messages.

"receive" -> "emit"

(the sink receives them from the channels (the source) but then drops them at the filter so they are not emitted outside the process)


v21.1/configure-logs.md, line 342 at r2 (raw file):

{{site.data.alerts.callout_success}}
In addition, the `DEV` channel should be output to a separate logging directory, since it is likely to contain sensitive data. See [`DEV` channel](#dev-channel).

Here and/or above: it would be useful to convey that we strongly recommend against pre-redaction for the DEV events, as this heavily impairs our ability to troubleshoot problems. The other channels do not need this recommendation.


v21.1/logging-overview.md, line 18 at r2 (raw file):

When a node processes a [`cockroach` command](cockroach-commands.html), it produces a stream of messages about the command's activities. Each message is composed of:

- A payload that contains notable events structured in JSON. For details on event types and their fields, see [Notable Event Types](eventlog.html).

The payload is not structured in JSON all the time. Only structured events have this property.
Non-structured events have an arbitrary string as payload with no discernible internal structure.


v21.1/logging-overview.md, line 21 at r2 (raw file):

- An envelope that contains event metadata (e.g., severity, date, timestamp, channel). Depending on the log format you specify when [configuring logs](configure-logs.html#file-logging-format), the envelope can be formatted either in JSON or as a flat prefix to the message.

Messages that meet a specified severity level are directed into appropriate [logging channels](#logging-channels) and can then be accessed via logging sinks, or output locations. The mapping of channels to sinks is configurable and is intended to support multiple [use cases](logging-use-cases.html). For more information on configuring sinks and severity levels, see [Configure Logs](configure-logs.html).

The filtering is done by the sink. The more accurate phrasing is thus:
"Messages are structured into logical logging channels, and then funneled through sinks that contain filtering and processing stages towards an output outside of the CockroachDB process. The mapping of channels to sinks, as well as the filtering and processing done by each sink, is configurable and ..."


v21.1/logging-use-cases.md, line 18 at r2 (raw file):

Most of the log entries record *structured* events

most of the log entries for non-DEV channels ...


v21.1/logging-use-cases.md, line 84 at r2 (raw file):

~~~
I210323 20:53:53.714286 232 2@server/status/runtime.go:553 ⋮ [n1] 1  runtime stats: 106 MiB RSS, 248 goroutines (stacks: 4.3 MiB), 28 MiB/51 MiB Go alloc/total (heap fragmentation: 5.2 MiB, heap reserved: 2.5 MiB, heap released: 24 MiB), 18 MiB/30 MiB CGO alloc/total (0.0 CGO/sec), 0.0/0.0 %%(u/s)time, 0.0 %%gc (0x), 46 KiB/107 KiB (r/w)net

This is not a structured event and thus we would prefer not to document it.

Arguably we probably do need a structured event here for this use case, but it's not implemented yet.

I would appreciate an issue on the crdb repo filed for this.


v21.1/logging-use-cases.md, line 95 at r2 (raw file):

~~~
I210402 01:24:38.576326 40084 2@rpc/nodedialer/nodedialer.go:160 ⋮ [n4] 283  unable to connect to n3: failed to connect to n3 at ‹localhost:26259›: ‹initial connection heartbeat failed›: ‹rpc error: code = Unavailable desc = connection error: desc = "transport: Error while dialing dial tcp: i/o timeout"›

This is not a structured event and thus we would prefer not to document it.

Arguably we probably do need a structured event here for this use case, but it's not implemented yet.

Also worth a (separate) issue.


v21.1/logging-use-cases.md, line 172 at r2 (raw file):

{{site.data.alerts.callout_info}}
In addition to SQL sessions, connection events can include SQL-based liveness probe attempts, as well as attempts to use the [PostgreSQL cancel protocol](https://www.postgresql.org/docs/current/protocol-flow.html#id-1.10.5.7.9).

IIRC we do not log an event for uses of the cancel protocol. It only increments a feature counter for telemetry.


v21.1/logging-use-cases.md, line 175 at r2 (raw file):

TLS certificate over TCP

TLS transport over TCP


v21.1/logging-use-cases.md, line 195 at r2 (raw file):

~~~

These logs show certificate authentication success over a `hostssl` (TLS certificate over TCP) connection: 

ditto


v21.1/logging-use-cases.md, line 208 at r2 (raw file):

- `User` shows that the SQL session is authenticated for user `roach`.

These logs show password authentication failure over a `hostssl` (TLS certificate over TCP) connection:

ditto

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @taroface and @thtruo)


v21.1/logging-use-cases.md, line 84 at r2 (raw file):

Previously, knz (kena) wrote…

This is not a structured event and thus we would prefer not to document it.

Arguably we probably do need a structured event here for this use case, but it's not implemented yet.

I would appreciate an issue on the crdb repo filed for this.

Filed: cockroachdb/cockroach#63750


v21.1/logging-use-cases.md, line 95 at r2 (raw file):

Previously, knz (kena) wrote…

This is not a structured event and thus we would prefer not to document it.

Arguably we probably do need a structured event here for this use case, but it's not implemented yet.

Also worth a (separate) issue.

Filed : cockroachdb/cockroach#63639

Copy link
Contributor Author

@taroface taroface left a comment

Choose a reason for hiding this comment

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

TFTR @knz! Had a couple of follow-up questions for you.

Also, still some outstanding items for me to resolve in this PR in a future commit.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz, @taroface, and @thtruo)


_includes/v21.1/misc/logging-flags.md, line 3 at r2 (raw file):

Previously, knz (kena) wrote…
All of the below settings are defined in the YAML payload.

"All the below settings can now be defined in the YAML payload instead."

Done.


v21.1/cockroach-gen.md, line 118 at r2 (raw file):

Previously, knz (kena) wrote…

Here and throughout: What about an include?

"By default, this command logs messages to...;

if you need to troubleshoot..."

Yes, good idea! Done. I left workload and demo alone, since they have different defaults.


v21.1/configure-logs.md, line 9 at r2 (raw file):

Previously, knz (kena) wrote…

nit: period at the end of each bullet point?

I think so, since each of these list items completes a sentence. Thanks!


v21.1/configure-logs.md, line 68 at r2 (raw file):

Previously, knz (kena) wrote…

"A logging sink represents one logical funnel through which the events from one or more logging channel are emitted externally to the cockroach process. Logging sinks are the unit of configurability for mapping events from inside to outside of CockroachDB."

How's this wording? I want to capture the basic gist while retaining accuracy.

"Logging sinks route events from specified logging channels to destinations outside CockroachDB."


v21.1/configure-logs.md, line 70 at r2 (raw file):

Previously, knz (kena) wrote…

here and below: it would be useful to convey the idea that the list of sink types will likely grow over time. Currently the phrasing suggests that there are only 3 types and that choice is closed forever.

I wasn't aware - made some tweaks that should convey this.


v21.1/configure-logs.md, line 89 at r2 (raw file):

Previously, knz (kena) wrote…

"All possible sink types .." (this will remain true even if we add more sink types)

Done. Using "supported" as the adjective.


v21.1/configure-logs.md, line 115 at r2 (raw file):

Previously, knz (kena) wrote…

nit: stray tab character, here and below.

Done.


v21.1/configure-logs.md, line 123 at r2 (raw file):

Previously, knz (kena) wrote…

name the log files

Done.


v21.1/configure-logs.md, line 159 at r2 (raw file):

Previously, knz (kena) wrote…

"The files generated for a group named default are named after the patterncockroach.XXXX.log"

(no need to mention the "channel" concept in this explanation)

Done.


v21.1/configure-logs.md, line 208 at r2 (raw file):

Previously, knz (kena) wrote…

nit: here and below; do we conventionally use contractions? if not, "it's" -> "it is"

According to our style guide, contractions can be used to simplify language. We don't use them when we are describing a clear directive or prohibition: https://github.com/cockroachdb/docs/wiki/Style-Guide#style-and-tone


v21.1/configure-logs.md, line 221 at r2 (raw file):

Previously, knz (kena) wrote…

What is this example coming from? I don't see the path cockroachdb/cockroach being mentioned elsewhere.

Ah, my mistake! This was my local path. Amended to /cockroach-data/logs.


v21.1/configure-logs.md, line 225 at r2 (raw file):

Previously, knz (kena) wrote…
in its own directory

"in the directory pointed to by its logging configuration".

(it's possible, albeit not recommended, to point multiple crdb processes to the same directory)

Done.


v21.1/configure-logs.md, line 262 at r2 (raw file):

Previously, knz (kena) wrote…

there are multiple JSON formats; how do you plan to explain that?

Added links to both json and json-compact.


v21.1/configure-logs.md, line 306 at r2 (raw file):

Previously, knz (kena) wrote…

"receive" -> "emit"

(the sink receives them from the channels (the source) but then drops them at the filter so they are not emitted outside the process)

Done. Thanks for clarifying this.


v21.1/configure-logs.md, line 342 at r2 (raw file):

Previously, knz (kena) wrote…

Here and/or above: it would be useful to convey that we strongly recommend against pre-redaction for the DEV events, as this heavily impairs our ability to troubleshoot problems. The other channels do not need this recommendation.

Done.


v21.1/logging-overview.md, line 18 at r2 (raw file):

Previously, knz (kena) wrote…

The payload is not structured in JSON all the time. Only structured events have this property.
Non-structured events have an arbitrary string as payload with no discernible internal structure.

Done.


v21.1/logging-overview.md, line 21 at r2 (raw file):

Previously, knz (kena) wrote…

The filtering is done by the sink. The more accurate phrasing is thus:
"Messages are structured into logical logging channels, and then funneled through sinks that contain filtering and processing stages towards an output outside of the CockroachDB process. The mapping of channels to sinks, as well as the filtering and processing done by each sink, is configurable and ..."

Trying this phrasing:

"Messages are organized into appropriate logging channels and then routed through logging sinks. Each sink further processes and filters the messages before emitting them to destinations outside CockroachDB. The mapping of channels to sinks, as well as the processing and filtering done by each sink..."


v21.1/logging-use-cases.md, line 18 at r2 (raw file):

Previously, knz (kena) wrote…
Most of the log entries record *structured* events

most of the log entries for non-DEV channels ...

Done.


v21.1/logging-use-cases.md, line 84 at r2 (raw file):

Previously, knz (kena) wrote…

Filed: cockroachdb/cockroach#63750

Thanks, sorry I didn't get to this sooner!

Is there a chance the structured events you describe in the two issues you filed will be ready for 21.1? If not, I'm not sure how to present this section. All I can think of is something like "If you are experiencing cluster issues and need help understanding these logs, please contact Support." That seems like it would look weird when we have examples everywhere else on this page.


v21.1/logging-use-cases.md, line 172 at r2 (raw file):

Previously, knz (kena) wrote…

IIRC we do not log an event for uses of the cancel protocol. It only increments a feature counter for telemetry.

Removed that part. It was left over from previous docs.


v21.1/logging-use-cases.md, line 175 at r2 (raw file):

Previously, knz (kena) wrote…
TLS certificate over TCP

TLS transport over TCP

Done.


v21.1/logging-use-cases.md, line 195 at r2 (raw file):

Previously, knz (kena) wrote…

ditto

Done.


v21.1/logging-use-cases.md, line 208 at r2 (raw file):

Previously, knz (kena) wrote…

ditto

Done.

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Reviewed 14 of 14 files at r3.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @taroface and @thtruo)


v21.1/configure-logs.md, line 221 at r2 (raw file):

Previously, taroface (Ryan Kuo) wrote…

Ah, my mistake! This was my local path. Amended to /cockroach-data/logs.

I think the leading / is still incorrect, the default would be cockroach-data/logs or ./cockroach-data/logs


v21.1/logging-overview.md, line 18 at r2 (raw file):

For details on event types and their fields

For details on structured event types ...


v21.1/logging-use-cases.md, line 84 at r2 (raw file):

Is there a chance the structured events you describe in the two issues you filed will be ready for 21.1?

Not for 21.1.0, probably for 21.1.1 or 21.1.2.

All I can think of is something like "If you are experiencing cluster issues and need help understanding these logs, please contact Support."

What about this:

  1. rename the section header from "Example" to "Content of log messages on this channel"

  2. explain:

"At the time of this writing, the events logged through this channel are unstructured. This means that Cockroach Labs does not yet place any guarantee on the specific format of the log entries and they can change between releases, including patch revisions. We assume that the entries are meant to be consumed by human operators."

You could add a mention that we are working on it

Copy link
Contributor

@thtruo thtruo left a comment

Choose a reason for hiding this comment

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

Nice @taroface! I read through the major new docs describing our logging system and configuration options which look good. It seems there are multiple comments you're addressing at the moment based off Raphael's feedback. I'm happy to review once a new iteration is posted.

Copy link
Contributor Author

@taroface taroface left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz and @taroface)


v21.1/configure-logs.md, line 221 at r2 (raw file):

Previously, knz (kena) wrote…

I think the leading / is still incorrect, the default would be cockroach-data/logs or ./cockroach-data/logs

Done.


v21.1/logging-overview.md, line 18 at r2 (raw file):

Previously, knz (kena) wrote…
For details on event types and their fields

For details on structured event types ...

Done.


v21.1/logging-use-cases.md, line 84 at r2 (raw file):

Previously, knz (kena) wrote…

Is there a chance the structured events you describe in the two issues you filed will be ready for 21.1?

Not for 21.1.0, probably for 21.1.1 or 21.1.2.

All I can think of is something like "If you are experiencing cluster issues and need help understanding these logs, please contact Support."

What about this:

  1. rename the section header from "Example" to "Content of log messages on this channel"

  2. explain:

"At the time of this writing, the events logged through this channel are unstructured. This means that Cockroach Labs does not yet place any guarantee on the specific format of the log entries and they can change between releases, including patch revisions. We assume that the entries are meant to be consumed by human operators."

You could add a mention that we are working on it

Done. Added a short callout to convey this.

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

:lgtm:

(I think you forgot to add the new callout file to the commit.)

Reviewed 18 of 18 files at r4.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @mikeCRL and @thtruo)

@github-actions
Copy link

github-actions bot commented May 7, 2021

Files changed:

_includes/sidebar-data-v21.1.json
_includes/v21.1/misc/logging-defaults.md
_includes/v21.1/misc/logging-flags.md
releases/v21.1.0-alpha.2.md
v21.1/cluster-setup-troubleshooting.md
v21.1/cockroach-cert.md
v21.1/cockroach-debug-merge-logs.md
v21.1/cockroach-debug-zip.md
v21.1/cockroach-demo.md
v21.1/cockroach-dump.md
v21.1/cockroach-gen.md
v21.1/cockroach-init.md
v21.1/cockroach-node.md
v21.1/cockroach-quit.md
v21.1/cockroach-sql.md
v21.1/cockroach-start-single-node.md
v21.1/cockroach-start.md
v21.1/cockroach-statement-diag.md
v21.1/cockroach-workload.md
v21.1/common-errors.md
v21.1/configure-logs.md
v21.1/dbeaver.md
v21.1/debug-and-error-logs.md
v21.1/developer-guide-overview.md
v21.1/encryption.md
v21.1/eventlog.md
v21.1/experimental-audit.md
v21.1/file-an-issue.md
v21.1/intellij-idea.md
v21.1/logformats.md
v21.1/logging-overview.md
v21.1/logging-use-cases.md
v21.1/logging.md
v21.1/logsinks.md
v21.1/query-behavior-troubleshooting.md
v21.1/recommended-production-settings.md
v21.1/show-trace.md
v21.1/sql-audit-logging.md
v21.1/sql-faqs.md
v21.1/stream-data-out-of-cockroachdb-using-changefeeds.md
v21.1/troubleshooting-overview.md
v21.1/ui-debug-pages.md

1 similar comment
@github-actions
Copy link

github-actions bot commented May 7, 2021

Files changed:

_includes/sidebar-data-v21.1.json
_includes/v21.1/misc/logging-defaults.md
_includes/v21.1/misc/logging-flags.md
releases/v21.1.0-alpha.2.md
v21.1/cluster-setup-troubleshooting.md
v21.1/cockroach-cert.md
v21.1/cockroach-debug-merge-logs.md
v21.1/cockroach-debug-zip.md
v21.1/cockroach-demo.md
v21.1/cockroach-dump.md
v21.1/cockroach-gen.md
v21.1/cockroach-init.md
v21.1/cockroach-node.md
v21.1/cockroach-quit.md
v21.1/cockroach-sql.md
v21.1/cockroach-start-single-node.md
v21.1/cockroach-start.md
v21.1/cockroach-statement-diag.md
v21.1/cockroach-workload.md
v21.1/common-errors.md
v21.1/configure-logs.md
v21.1/dbeaver.md
v21.1/debug-and-error-logs.md
v21.1/developer-guide-overview.md
v21.1/encryption.md
v21.1/eventlog.md
v21.1/experimental-audit.md
v21.1/file-an-issue.md
v21.1/intellij-idea.md
v21.1/logformats.md
v21.1/logging-overview.md
v21.1/logging-use-cases.md
v21.1/logging.md
v21.1/logsinks.md
v21.1/query-behavior-troubleshooting.md
v21.1/recommended-production-settings.md
v21.1/show-trace.md
v21.1/sql-audit-logging.md
v21.1/sql-faqs.md
v21.1/stream-data-out-of-cockroachdb-using-changefeeds.md
v21.1/troubleshooting-overview.md
v21.1/ui-debug-pages.md

@github-actions
Copy link

Files changed:

_includes/sidebar-data-v21.1.json
_includes/v21.1/misc/logging-defaults.md
_includes/v21.1/misc/logging-flags.md
releases/v21.1.0-alpha.2.md
v21.1/cluster-setup-troubleshooting.md
v21.1/cockroach-cert.md
v21.1/cockroach-debug-merge-logs.md
v21.1/cockroach-debug-zip.md
v21.1/cockroach-demo.md
v21.1/cockroach-dump.md
v21.1/cockroach-gen.md
v21.1/cockroach-init.md
v21.1/cockroach-node.md
v21.1/cockroach-quit.md
v21.1/cockroach-sql.md
v21.1/cockroach-start-single-node.md
v21.1/cockroach-start.md
v21.1/cockroach-statement-diag.md
v21.1/cockroach-workload.md
v21.1/common-errors.md
v21.1/configure-logs.md
v21.1/dbeaver.md
v21.1/debug-and-error-logs.md
v21.1/developer-guide-overview.md
v21.1/encryption.md
v21.1/eventlog.md
v21.1/experimental-audit.md
v21.1/file-an-issue.md
v21.1/intellij-idea.md
v21.1/logformats.md
v21.1/logging-overview.md
v21.1/logging-use-cases.md
v21.1/logging.md
v21.1/logsinks.md
v21.1/query-behavior-troubleshooting.md
v21.1/recommended-production-settings.md
v21.1/show-trace.md
v21.1/sql-audit-logging.md
v21.1/sql-faqs.md
v21.1/stream-data-out-of-cockroachdb-using-changefeeds.md
v21.1/troubleshooting-overview.md
v21.1/ui-debug-pages.md

Copy link
Contributor

@mikeCRL mikeCRL left a comment

Choose a reason for hiding this comment

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

Great work, here! Left some comments/suggestions.

`--no-color` | Do not colorize `stderr`. Possible values: `true` or `false`.<br/><br/>When set to `false`, messages logged to `stderr` are colorized based on [severity level](debug-and-error-logs.html#severity-levels). <br><br>**Default:** `false`
`--sql-audit-dir` | If non-empty, create a SQL audit log in this directory. By default, SQL audit logs are written in the same directory as the other logs generated by CockroachDB.<br><br>Note that enabling SQL audit logs can negatively impact performance. As a result, we recommend using SQL audit logs for security purposes only. For more information, see the [`EXPERIMENTAL_AUDIT`](experimental-audit.html) reference page.
`--log` | <span class="version-tag">New in v21.1:</span> Configure logging parameters, in conjunction with a YAML payload. For details, see [Configure logs](configure-logs.html). If not specified, the [default configuration](configure-logs.html#default-logging-configuration) is used.<br/><br/>**Note:** The deprecated logging flags below cannot be combined with `--log`. All of the below settings can be defined instead in the YAML payload.
`--log-dir` | **Deprecated.** To enable logging to files and write logs to the specified directory, use `--log` and set `dir` in the YAML configuration. <br/><br/>Setting `--log-dir` to a blank directory (`--log-dir=""`) disables logging to files.
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding a phrase to this and the other deprecated --log* flag descriptions like "This is now handled by the log configuration YAML specified with --log." Or if not applicable, some other detail about its current state/effect/lack thereof.

@@ -30,7 +30,7 @@ To resolve this issue, do one of the following:
- If the node hasn't yet been started, [start the node](cockroach-start.html).
- If you specified a [`--listen-addr` and/or a `--advertise-addr` flag](cockroach-start.html#networking) when starting the node, you must include the specified IP address/hostname and port with all other [`cockroach` commands](cockroach-commands.html) or change the `COCKROACH_HOST` environment variable.

If you're not sure what the IP address/hostname and port values might have been, you can look in the node's [logs](debug-and-error-logs.html). If necessary, you can also terminate the `cockroach` process, and then restart the node:
If you're not sure what the IP address/hostname and port values might have been, you can look in the node's [logs](logging-overview.html). If necessary, you can also terminate the `cockroach` process, and then restart the node:
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
If you're not sure what the IP address/hostname and port values might have been, you can look in the node's [logs](logging-overview.html). If necessary, you can also terminate the `cockroach` process, and then restart the node:
If you're not sure what the IP address/hostname and port values might have been, you can look in the node's [logs](logging-overview.html). If necessary, you can also terminate the `cockroach` process and then restart the node:

For examples of how these settings can be used in practice, see [Logging Use Cases](logging-use-cases.html).

{{site.data.alerts.callout_info}}
The logging flags previously used with `cockroach` commands are now deprecated. Instead, use the YAML definitions as described below. The [default logging configuration](#default-logging-configuration) uses YAML settings that are backward-compatible with v20.2 and earlier.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what it means here to be backward compatible with an earlier version if we're talking about recording new logs, and the new feature requires v21.1. Maybe this could be clarified.

{{site.data.alerts.end}}

{{site.data.alerts.callout_success}}
You can view your current settings by running `cockroach debug check-log-config`, which returns the YAML definitions and a URL to a visualization of the current logging configuration.
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
You can view your current settings by running `cockroach debug check-log-config`, which returns the YAML definitions and a URL to a visualization of the current logging configuration.
You can view your current settings by running `cockroach debug check-log-config`, which returns the YAML definitions and a URL for visualizing the current logging configuration.

@@ -0,0 +1,7 @@
---
title: Logging Sinks
Copy link
Contributor

Choose a reason for hiding this comment

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

Log Sinks seems more common. Consider renaming?

Regardless, I believe the file name for the page should have a hyphen per our convention.

## SQL logging
## Local query testing

If you are testing CockroachDB locally and want to log queries executed just by a specific node, you can either pass a CLI flag at node startup, or execute a SQL function on a running node.
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
If you are testing CockroachDB locally and want to log queries executed just by a specific node, you can either pass a CLI flag at node startup, or execute a SQL function on a running node.
If you are testing CockroachDB locally and want to log queries executed just by a specific node, you can either pass a CLI flag at node startup or execute a SQL function on a running node.


If you are testing CockroachDB locally and want to log queries executed just by a specific node, you can either pass a CLI flag at node startup, or execute a SQL function on a running node.

Using the CLI to start a new node, pass the `--vmodule` flag to the [`cockroach start`](cockroach-start.html) command. For example, to start a single node locally and log all client-generated SQL queries it executes, you'd run:
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think of it as passing a flag to the command but more about passing data/information to the application/script using a flag 'on/with' the command. This is not authoritative so take with a grain of salt.

Suggested change
Using the CLI to start a new node, pass the `--vmodule` flag to the [`cockroach start`](cockroach-start.html) command. For example, to start a single node locally and log all client-generated SQL queries it executes, you'd run:
Using the CLI to start a new node, use the `--vmodule` flag with the [`cockroach start`](cockroach-start.html) command. For example, to start a single node locally and log all client-generated SQL queries it executes, you'd run:

{
"title": "Log Formats",
"urls": [
"/${VERSION}/logformats.html"
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider renaming to log-sinks.html to match our other pages.

(Same with logformats.html as I mentioned elsewhere.)

toc: true
---

{% remote_include https://raw.githubusercontent.com/cockroachdb/cockroach/master/docs/generated/eventlog.md %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let me know if you'd like me to go back and review the include. I skimmed.

FYI in this one I noticed 22 cases where we lack a description, in case you want to ensure there is a cockroach repo issue for improving this.

toc: true
---

{% remote_include https://raw.githubusercontent.com/cockroachdb/cockroach/master/docs/generated/logging.md %}
Copy link
Contributor

Choose a reason for hiding this comment

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

We could link SENSITIVE_ACCESS (in this remote_include) back to experimental-audit.md if appropriate. There may be other opportunities to go back to the generated docs and add links back to the new docs, as a follow-up task, post-release.

Copy link
Contributor

@thtruo thtruo left a comment

Choose a reason for hiding this comment

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

Thanks @taroface - barring final touches mentioned in the latest round of comments, this latest iteration generally looks good to me!

The overall structure is sound, content is easy to parse, and included essential callouts around specific configuration UX (e.g. using shell redirection syntax to pass in yaml files directly)

This LGTM

Copy link
Contributor Author

@taroface taroface left a comment

Choose a reason for hiding this comment

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

TFTRs Mike and Tommy!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @knz and @mikeCRL)


_includes/sidebar-data-v21.1.json, line 2748 at r7 (raw file):

Previously, mikeCRL (Mike Lewis) wrote…

Consider renaming to log-sinks.html to match our other pages.

(Same with logformats.html as I mentioned elsewhere.)

Done.


_includes/v21.1/misc/logging-flags.md, line 4 at r7 (raw file):

Previously, mikeCRL (Mike Lewis) wrote…

Consider adding a phrase to this and the other deprecated --log* flag descriptions like "This is now handled by the log configuration YAML specified with --log." Or if not applicable, some other detail about its current state/effect/lack thereof.

Done.


v21.1/common-errors.md, line 33 at r7 (raw file):

Previously, mikeCRL (Mike Lewis) wrote…
If you're not sure what the IP address/hostname and port values might have been, you can look in the node's [logs](logging-overview.html). If necessary, you can also terminate the `cockroach` process and then restart the node:

Done.


v21.1/configure-logs.md, line 17 at r7 (raw file):

Previously, mikeCRL (Mike Lewis) wrote…

I'm not sure what it means here to be backward compatible with an earlier version if we're talking about recording new logs, and the new feature requires v21.1. Maybe this could be clarified.

Done.


v21.1/configure-logs.md, line 63 at r7 (raw file):

Previously, mikeCRL (Mike Lewis) wrote…
You can view your current settings by running `cockroach debug check-log-config`, which returns the YAML definitions and a URL for visualizing the current logging configuration.

I'm going to keep the original wording for this one, as I think it is more accurate (if somewhat a mouthful).


v21.1/eventlog.md, line 7 at r7 (raw file):

Previously, mikeCRL (Mike Lewis) wrote…

Let me know if you'd like me to go back and review the include. I skimmed.

FYI in this one I noticed 22 cases where we lack a description, in case you want to ensure there is a cockroach repo issue for improving this.

Will make a note of this.


v21.1/logging.md, line 7 at r7 (raw file):

Previously, mikeCRL (Mike Lewis) wrote…

We could link SENSITIVE_ACCESS (in this remote_include) back to experimental-audit.md if appropriate. There may be other opportunities to go back to the generated docs and add links back to the new docs, as a follow-up task, post-release.

Will make a note of this.


v21.1/logsinks.md, line 2 at r7 (raw file):

Previously, mikeCRL (Mike Lewis) wrote…

Log Sinks seems more common. Consider renaming?

Regardless, I believe the file name for the page should have a hyphen per our convention.

Done.


v21.1/query-behavior-troubleshooting.md, line 148 at r7 (raw file):

Previously, mikeCRL (Mike Lewis) wrote…
If you are testing CockroachDB locally and want to log queries executed just by a specific node, you can either pass a CLI flag at node startup or execute a SQL function on a running node.

Done.


v21.1/query-behavior-troubleshooting.md, line 150 at r7 (raw file):

Previously, mikeCRL (Mike Lewis) wrote…

I don't think of it as passing a flag to the command but more about passing data/information to the application/script using a flag 'on/with' the command. This is not authoritative so take with a grain of salt.

Using the CLI to start a new node, use the `--vmodule` flag with the [`cockroach start`](cockroach-start.html) command. For example, to start a single node locally and log all client-generated SQL queries it executes, you'd run:

Done.

@github-actions
Copy link

Files changed:

_includes/sidebar-data-v21.1.json
_includes/v21.1/misc/logging-defaults.md
_includes/v21.1/misc/logging-flags.md
releases/v21.1.0-alpha.2.md
v21.1/cluster-setup-troubleshooting.md
v21.1/cockroach-cert.md
v21.1/cockroach-debug-merge-logs.md
v21.1/cockroach-debug-zip.md
v21.1/cockroach-demo.md
v21.1/cockroach-dump.md
v21.1/cockroach-gen.md
v21.1/cockroach-init.md
v21.1/cockroach-node.md
v21.1/cockroach-quit.md
v21.1/cockroach-sql.md
v21.1/cockroach-start-single-node.md
v21.1/cockroach-start.md
v21.1/cockroach-statement-diag.md
v21.1/cockroach-workload.md
v21.1/common-errors.md
v21.1/configure-logs.md
v21.1/dbeaver.md
v21.1/debug-and-error-logs.md
v21.1/developer-guide-overview.md
v21.1/encryption.md
v21.1/eventlog.md
v21.1/experimental-audit.md
v21.1/file-an-issue.md
v21.1/intellij-idea.md
v21.1/logging-overview.md
v21.1/logging-use-cases.md
v21.1/logging.md
v21.1/query-behavior-troubleshooting.md
v21.1/recommended-production-settings.md
v21.1/show-trace.md
v21.1/sql-audit-logging.md
v21.1/sql-faqs.md
v21.1/stream-data-out-of-cockroachdb-using-changefeeds.md
v21.1/troubleshooting-overview.md
v21.1/ui-debug-pages.md

@github-actions
Copy link

Files changed:

_includes/sidebar-data-v21.1.json
_includes/v21.1/misc/logging-defaults.md
_includes/v21.1/misc/logging-flags.md
releases/v21.1.0-alpha.2.md
v21.1/cluster-setup-troubleshooting.md
v21.1/cockroach-cert.md
v21.1/cockroach-debug-merge-logs.md
v21.1/cockroach-debug-zip.md
v21.1/cockroach-demo.md
v21.1/cockroach-dump.md
v21.1/cockroach-gen.md
v21.1/cockroach-init.md
v21.1/cockroach-node.md
v21.1/cockroach-quit.md
v21.1/cockroach-sql.md
v21.1/cockroach-start-single-node.md
v21.1/cockroach-start.md
v21.1/cockroach-statement-diag.md
v21.1/cockroach-workload.md
v21.1/common-errors.md
v21.1/configure-logs.md
v21.1/dbeaver.md
v21.1/developer-guide-overview.md
v21.1/encryption.md
v21.1/eventlog.md
v21.1/experimental-audit.md
v21.1/file-an-issue.md
v21.1/intellij-idea.md
v21.1/log-formats.md
v21.1/log-sinks.md
v21.1/logging-overview.md
v21.1/logging-use-cases.md
v21.1/logging.md
v21.1/query-behavior-troubleshooting.md
v21.1/recommended-production-settings.md
v21.1/show-trace.md
v21.1/sql-audit-logging.md
v21.1/sql-faqs.md
v21.1/stream-data-out-of-cockroachdb-using-changefeeds.md
v21.1/troubleshooting-overview.md
v21.1/ui-debug-pages.md

@taroface taroface merged commit 775ee1b into master May 12, 2021
@taroface taroface deleted the logging branch May 12, 2021 17:36
This was referenced May 12, 2021
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.

5 participants