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

docs: new RFC on logging modernization #51990

Merged
merged 1 commit into from
Dec 30, 2020
Merged

Conversation

knz
Copy link
Contributor

@knz knz commented Jul 28, 2020

@knz knz requested a review from a team as a code owner July 28, 2020 13:12
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@thtruo
Copy link
Contributor

thtruo commented Jul 28, 2020

Thanks @knz. This might be out of scope for this RFC, but what is the job of the system.event_log in the grand scheme of this new logging infrastructure? Do we have any guidance around what should be tracked in system.event_log vs other (future) logging channels?

For example, today customers request getting access to DDL events in their audit logs. However, DDL events are not recorded in audit logs so customers workaround that by querying system.event_log.

@knz
Copy link
Contributor Author

knz commented Jul 28, 2020

I don't have an opinion about keeping system.event_log or not once its contents are also copied to the OPS, USER_ADMIN and SENSITIVE_ACCESS logging channels.

There are arguments either way.
The same discussion also applies to system.range_log.

craig bot pushed a commit that referenced this pull request Aug 6, 2020
52200: util/log: improvements, simplifications and bug fixes r=irfansharif a=knz

Fixes #52026
Fixes #52176
Fixes #52128 
Fixes #51332 
Supersedes #51499, #51410 

For context, I was working on #51990, #51499 and #51410  when I stumbled upon bug #52128 in the logging package. I decided to clean up the code and fix that bug, and I ended up removing a lot of complexity and add many missing comments.

Example CI output:
![image](https://user-images.githubusercontent.com/642886/89288431-efaedc80-d655-11ea-97d9-b096b580d951.png)


By removing the complexity, I removed the problem otherwise solved by  #51499 & #51410 and  I removed the source of the flake causing #52026 and #52176.

Salient bits in this PR:

- **util/log: reinforce the behavior of log file rotation** - "This patch "does the right thing" by opening the new file before switching over whatever redirection may have been set up from the old file to the new one."
- **util/log: simplify TestLogScope+don't capture internal stderr writes** 
  - "This complexity was thus removed and the doc clarified that TestLogScope is only valid under the default configuration, when there was no file output configured yet and no secondary loggers active yet."
  - "However we also have other reasons to dislike the stderr redirect in tests, so I decided to remove it here instead of conjuring new code."
- **pgwire: un-skip TestauthenticationAndHBARules**


Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
@knz
Copy link
Contributor Author

knz commented Dec 30, 2020

Merging this as "completed" since the work was, in fact, completed.

bors r+

@craig
Copy link
Contributor

craig bot commented Dec 30, 2020

Build failed:

@knz
Copy link
Contributor Author

knz commented Dec 30, 2020

bors r+

@knz
Copy link
Contributor Author

knz commented Dec 30, 2020

(Split all the eventlog-related changes into a different RFC, see #58374)

@craig craig bot merged commit ad3dea8 into cockroachdb:master Dec 30, 2020
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.

3 participants