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

Clarify logging documentation #21665

Merged
merged 11 commits into from
Nov 22, 2022
Merged

Clarify logging documentation #21665

merged 11 commits into from
Nov 22, 2022

Conversation

mpeter50
Copy link
Contributor

@mpeter50 mpeter50 commented Nov 2, 2022

My pull request changes the logging documentation that is visible here: https://docs.gitea.io/en-us/logging-configuration/
The reason behind the changes is that for some time I've found the logging documentation confusing, and wanted to give a try at making it more clear.


If you find the existing changes to be ok, please don't merge yet, as I have further ideas which I want to discuss with you before making the changes.

Swap the "Log Groups" and "Log outputs" sections.

I want to move the "Log outputs" section before the "Log Groups" section. The reason is that the "Log Groups" section refers to ini sections that are only later explained, and to concepts that are general and should be documented in "Log outputs" or a different section.

This change is essentially a swap of the "Log Groups" and "Log outputs" sections. That way the doumentation would follow the structure in which the ini file is built: first explaining the outer sections, and then the inner ones ([log], [log.name], [log.name.default], ...)

Explain the workings of ambigous settings below the settings listing

Right now the basics of a setting is shown later than the explanation of its special workings, for example with FILE_NAME at the file output mode (well, if the first changes are taken into account).

Currently I have TODO witten at 2 settings, which I have to figure out how do they exactly work before I can document them.

New section about [log]

New section after "Collecting Logs for Help" about how the top level [log] itself works and what can go there.
Currently, variables that directly go into [log] are noted throughout the whole document.


Please let me know what you think about the changes.

A counterargument that I myself see is that some of this is already present in the cheatsheet, but I think it would be better to have this document as a throrough explanation of how logging is configured, and the cheatsheet would only have a short outline of the possible sections and variables.

extend documentation of logger outputs with short description (what do they do), and fix mistake in the brief listing
…nfig values.

The new order of the settings moves those to earlier places that have a higher impact over the functionality of the logger
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Nov 3, 2022
@lunny lunny added the type/docs This PR mainly updates/creates documentation label Nov 3, 2022
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 4, 2022
Copy link
Member

@yardenshoham yardenshoham left a comment

Choose a reason for hiding this comment

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

You have some markdownlint errors

docs/content/doc/advanced/logging-documentation.en-us.md Outdated Show resolved Hide resolved
docs/content/doc/advanced/logging-documentation.en-us.md Outdated Show resolved Hide resolved
docs/content/doc/advanced/logging-documentation.en-us.md Outdated Show resolved Hide resolved
docs/content/doc/advanced/logging-documentation.en-us.md Outdated Show resolved Hide resolved
@mpeter50
Copy link
Contributor Author

mpeter50 commented Nov 7, 2022

Oh, we have a Markdown linter, thanks for reminding me about it! Pushed the fix.

Also, I'll go ahead with the other changes.
In the meantime I'll mark this PR as WIP so it doesn't get merged before I finish everything. Forgot to do that earlier.

@mpeter50 mpeter50 changed the title Clarify logging documentation [WIP] Clarify logging documentation Nov 7, 2022
@mpeter50 mpeter50 marked this pull request as draft November 7, 2022 18:54
@mpeter50 mpeter50 changed the title [WIP] Clarify logging documentation Clarify logging documentation Nov 7, 2022
Details the usage of the main [log] section, and the default values it uses.
Outlines how the logging configuration is built up
@mpeter50
Copy link
Contributor Author

mpeter50 commented Nov 7, 2022

For now if everything is ok then this is the final form of the file.

There is still one TODO in it, though: I want to clarify what LOG_ROTATE being set to true without DAILY_ROTATE being set to true results in. How will rotation work in this case?

  • Will it delete old lines, empty the file, or do something else when the maximum size is reached?
  • Does MAX_DAYS have an effect?

I'm not familiar with the codebase, and searching for rotate (as seen here) did not reveal where is it used.
Could you please help in finding these out?

@mpeter50 mpeter50 requested review from yardenshoham and lunny and removed request for yardenshoham November 15, 2022 12:51
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@6c8ff32). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main   #21665   +/-   ##
=======================================
  Coverage        ?   48.21%           
=======================================
  Files           ?     1031           
  Lines           ?   140846           
  Branches        ?        0           
=======================================
  Hits            ?    67912           
  Misses          ?    64817           
  Partials        ?     8117           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mpeter50
Copy link
Contributor Author

I've just noticed that in the same event where I requested reviews, Github says this:

and removed request for yardenshoham

Does that mean that I misclicked something, or that @yardenshoham has approved the changes (as the reviewers section on the right says)?

@yardenshoham
Copy link
Member

Generally, if you want your PR reviewed change it from draft PR

@mpeter50
Copy link
Contributor Author

Generally, if you want your PR reviewed change it from draft PR

Oh, ok, thanks. I've set it this way so it's not merged accidentally, but I'll remove the draft marking.

@mpeter50 mpeter50 marked this pull request as ready for review November 18, 2022 23:25
@lunny lunny merged commit 371dd96 into go-gitea:main Nov 22, 2022
fsologureng pushed a commit to fsologureng/gitea that referenced this pull request Nov 22, 2022
My pull request changes the logging documentation that is visible here:
https://docs.gitea.io/en-us/logging-configuration/
The reason behind the changes is that for some time I've found the
logging documentation confusing, and wanted to give a try at making it
more clear.

---

If you find the existing changes to be ok, please don't merge yet, as I
have further ideas which I want to discuss with you before making the
changes.

### Swap the "Log Groups" and "Log outputs" sections.
I want to move the "Log outputs" section before the "Log Groups"
section. The reason is that the "Log Groups" section refers to ini
sections that are only later explained, and to concepts that are general
and should be documented in "Log outputs" or a different section.

This change is essentially a swap of the "Log Groups" and "Log outputs"
sections. That way the doumentation would follow the structure in which
the ini file is built: first explaining the outer sections, and then the
inner ones ([log], [log.name], [log.name.default], ...)

### Explain the workings of ambigous settings below the settings listing
Right now the basics of a setting is shown later than the explanation of
its special workings, for example with `FILE_NAME` at [the file output
mode](https://docs.gitea.io/en-us/logging-configuration/#file-mode)
(well, if the first changes are taken into account).

Currently I have `TODO` witten at 2 settings, which I have to figure out
how do they exactly work before I can document them.

### New section about [log]
New section after "Collecting Logs for Help" about how the top level
[log] itself works and what can go there.
Currently, variables that directly go into [log] are noted throughout
the whole document.

---

Please let me know what you think about the changes.

A counterargument that I myself see is that some of this is already
present in the cheatsheet, but I think it would be better to have [this
document](https://docs.gitea.io/en-us/logging-configuration/) as a
throrough explanation of how logging is configured, and the cheatsheet
would only have a short outline of the possible sections and variables.

Co-authored-by: Lunny Xiao <[email protected]>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Nov 23, 2022
* giteaofficial/main:
  Handle empty author names (go-gitea#21902)
  Move all remaining colors into CSS variables (go-gitea#21903)
  Add option to enable CAPTCHA validation for login (go-gitea#21638)
  Prepend refs/heads/ to issue template refs (go-gitea#20461)
  Fixes go-gitea#21895: standardize UTC tz for util tests (go-gitea#21897)
  Clarify logging documentation (go-gitea#21665)
  Update JS dependencies (go-gitea#21881)
  Webhook list enhancements (go-gitea#21893)
  Embed Matrix icon as SVG (go-gitea#21890)
  fix(web): add `alt` for logo in home page (go-gitea#21887)
  Improvements for Content Copy (go-gitea#21842)
  Replace yaml.v2 with yaml.v3 (go-gitea#21832)
  Allow disable RSS/Atom feed (go-gitea#21622)
  Consolidate security-check into checks-backend (go-gitea#21882)
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/docs This PR mainly updates/creates documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants