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

log: add docs on when new logging channels are warranted #74770

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dhartunian
Copy link
Collaborator

Motivated by #74404 and #74699

Release note: None

@dhartunian dhartunian requested review from knz, joshimhoff, tbg and a team January 12, 2022 21:58
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@dhartunian dhartunian force-pushed the add-guidelines-on-adding-new-logging-channels branch from eea4feb to c512d70 Compare January 12, 2022 21:59
Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

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

These sound convincing to me, but I am a pure consumer here - I have not formed opinions on logging channels and when to introduce them, so better to wait for a second review.

@@ -0,0 +1,35 @@
# When to create a new logging channel
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a link to this doc from the channel enum proto in channels.proto

- For instance: `SQL_SCHEMA` is a channel that groups together
all logs pertaining to schema changes. This is helpful because
this information could otherwise be scattered across many
different logs.
Copy link
Contributor

Choose a reason for hiding this comment

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

"... different log entries with different structures, which would make it hard to filter from a shared channel (e.g. the DEV channel)."

logging infrastructure already provides tooling for
differentiating between different sources or categorizations of
logs.)
- For instance: `USER_ADMIN` and `PRIVILEGES` are likely to be
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good example for your point, but then the reader would immediately ask: Why two separate channels USER_ADMIN and PRIVILEGES? Doesn't look that they would be routed differently?

It's worth explaining that the reason those two in particular are separate from one another is because of point 3 below: we're expecting USER_ADMIN to out-crowd PRIVILEGES but we don't want to lose data from the latter due to file rotations.

Copy link
Contributor

@lidorcarmel lidorcarmel left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian and @joshimhoff)


pkg/util/log/logpb/when_to_create_a_new_channel.md line 19 at r1 (raw file):

   - For instance: `SQL_SCHEMA` is a channel that groups together 
     all logs pertaining to schema changes. This is helpful because 
     this information could otherwise be scattered across many 

why across many different logs? without a separate channel we will have everything in the main (DEV) channel, no?

Code quote:

     this information could otherwise be scattered across many
     different logs.

- For instance: `USER_ADMIN` and `PRIVILEGES` are likely to be
important for security audit purposes and exist to help with
routing to a security team, rather than a DB operations team.
3. There is a pattern of logging traffic that is at risk of
Copy link
Contributor

@angles-n-daemons angles-n-daemons Oct 2, 2024

Choose a reason for hiding this comment

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

Are logs generally used in the raw files? Is there some expectation that they end up in a search index before usage?

all logs pertaining to schema changes. This is helpful because
this information could otherwise be scattered across many
different logs.
2. There is a strong need for _routing_ a subset of logging
Copy link
Contributor

@angles-n-daemons angles-n-daemons Oct 2, 2024

Choose a reason for hiding this comment

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

I agree pretty strongly with this. I see a primary use case for channels to specify a destination for logs (DD Index, ES, etc) more than anything else.

@@ -0,0 +1,35 @@
# When to create a new logging channel

In general, we should avoid creating new logging channels.
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing that's missing here is if you'd like to apply post-processing to a subset of logs. Consider encryption for example, perhaps there's a log channel with particularly sensitive information. In this case it makes sense to separate a channels so that specific post-processing can be applied to the log files, via sink or data-processing pipeline.

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.

6 participants