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

logs: cockroach check debug-check-config does not match user created yaml configuration #59048

Open
thtruo opened this issue Jan 15, 2021 · 2 comments
Labels
A-kv-server Relating to the KV-level RPC server A-logging In and around the logging infrastructure. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-observability

Comments

@thtruo
Copy link
Contributor

thtruo commented Jan 15, 2021

Observation

I have a logs.yaml configuration that looks like this:

file-defaults:
  dir: cockroach-data/logs 
  filter: INFO             
  redact: false            
  redactable: false      
  max-file-size: 10MB 
  max-group-size: 100MB  
  format: json             
sinks:
 file-groups:              
   security:               # cockroach-security.log
     channels: "SESSIONS, USER_ADMIN, PRIVILEGES, SENSITIVE_ACCESS"
     format: json
   health:                 # cockroach-health.log
     channels: "HEALTH, OPS"
     format: json
   devonly:                # cockroach-devonly.log 👈
     channels: "DEV"
     format: json
 stderr:
   channels: "DEV"         # default is ALL
   format: json
   filter: INFO            # filter: NONE disables stderr by default (this is the default config)

Note that I have a line that only pushes DEV channel events into a file called cockroach-devonly.log.

However, cockroach check debug-check-config --log="$(cat logs.yaml)" looks like this:

Expectation
I expected only the DEV channel events would go into cockroach-devonly.log. It caught me off guard to see SQL_* and STORAGE channel events go into cockroach-devonly.log. My YAML configuration explicitly includes one channel (DEV).
Image 2021-01-15 at 11 14 25 AM

cc @knz again let me know if I'm making a user error or if this is indeed expected
cc @piyush-singh for awareness

Jira issue: CRDB-3340

@thtruo thtruo added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-kv-server Relating to the KV-level RPC server labels Jan 15, 2021
@knz knz added this to To do in DB Server & Security via automation Jan 15, 2021
@knz
Copy link
Contributor

knz commented Jan 15, 2021

The current logic automatically takes all the channels not otherwise listed by the user's configuration, and redirects them to whatever sink the user configured for the DEV channel.

The motivation behind this choice is to have a natural "catch-all" whenever we add new channels in CockroachDB in the future, to avoid having these entries lost altogether.

I think I understand the confusion however. What behavior would you suggest instead, that reduces the confusion but also keeps a "catch-all" behavior for future, yet-unknown channels?

@thtruo
Copy link
Contributor Author

thtruo commented Jan 15, 2021

The current logic automatically takes all the channels not otherwise listed by the user's configuration, and redirects them to whatever sink the user configured for the DEV channel.

Ohh! Understood - I didn't pick up on that condition in my first go-around. That's clear now.

I get the motivation for having a "catch-all". My first impression was that it felt unnatural that unlisted channels get redirected to the same sink as the DEV channel, but I also get it. Definitely worth clarifying and calling out in docs cc @taroface

The only alternative behavior that comes to mind is making DEV channel events go into its own file unless it is listed as a channel in another sink. In a sense, DEV channel events will be like "stray errors"; where "stray errors" go into their own cockroach-stderr.log, DEV channel events go into their own cockroach-dev.log.

So if we follow that line of thinking, scenarios will play out so that unlisted channels will end up in the same "catchall" file. And then DEV ends up in its own file unless it is explicitly listed for another sink.

I think the merits of having DEV channel events go into their own file is to emphasize the unique characteristics of it, as you called out in your RFC:

  • The channel is used during CockroachDB development
  • This channel is special in that there are no constraints as to what may or may not be logged on it
  • These events are undocumented
  • And in this best practice section: do not centrally collect logs from the DEV channel ahead of scraping logs for data-mining purposes

Let me know if this suggestion runs into a blindspot. If it doesn't make sense to deviate from the current logic, that's okay too. Either way, it reinforces the need to clearly call this out in docs.

@knz knz moved this from To do to Linked issues (from the roadmap columns on the right) in DB Server & Security Jan 18, 2021
@jlinder jlinder added the T-server-and-security DB Server & Security label Jun 16, 2021
@knz knz added the A-logging In and around the logging infrastructure. label Jul 29, 2021
@thtruo thtruo added T-observability-inf and removed T-server-and-security DB Server & Security labels Feb 18, 2022
@jtsiros jtsiros added this to To do in Observability Infrastructure via automation Feb 22, 2022
@jtsiros jtsiros removed this from Linked issues (from the roadmap columns on the right) in DB Server & Security Feb 22, 2022
@knz knz removed their assignment Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-kv-server Relating to the KV-level RPC server A-logging In and around the logging infrastructure. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-observability
Projects
No open projects
Development

No branches or pull requests

3 participants