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.cfg{modules = ...} isn't equivalent to box.cfg{log_modules = ...} #7962

Closed
Gumix opened this issue Nov 22, 2022 · 3 comments · Fixed by #9290
Closed

log.cfg{modules = ...} isn't equivalent to box.cfg{log_modules = ...} #7962

Gumix opened this issue Nov 22, 2022 · 3 comments · Fixed by #9290
Assignees
Labels
2.11 Target is 2.11 and all newer release/master branches bug Something isn't working config logger Issues related to logging subsystem

Comments

@Gumix
Copy link
Contributor

Gumix commented Nov 22, 2022

#7672 allows to specify a log level for each module individually, however there is some inconsistency:

  • box.cfg{log_level_modules={...}} overrides the existing list of modules in log.cfg, while
  • log.cfg{level_modules={...}} appends the specified modules to the existing list.

Also it's not possible to remove a single module from the config by setting it to box.NULL.

gh_3211_per_module_log_level_test.lua contains the corresponding test-cases under TODO.

@nshy:

I would also check that resetting works if we first call log.cfg and then box.cfg (first box.cfg call, reset here smth from level_modules). I bet it will not work now. Unfortunately it is not easy to test with server:exec as it calls box.cfg on start.

The fix of this issue requires thorough changes in merge_cfg(), etc.

@Gumix Gumix added logger Issues related to logging subsystem 2.11 Target is 2.11 and all newer release/master branches labels Nov 22, 2022
@kyukhin kyukhin added 3.0 Target is 3.0 and all newer release/master branches and removed 2.11 Target is 2.11 and all newer release/master branches labels Jan 12, 2023
@Totktonada
Copy link
Member

Please, glance on #8610 as well.

@locker locker self-assigned this Oct 20, 2023
@locker
Copy link
Member

locker commented Oct 20, 2023

  • box.cfg{log_level_modules={...}} overrides the existing list of modules in log.cfg, while
  • log.cfg{level_modules={...}} appends the specified modules to the existing list.

I think that we should make log.cfg{modules = ...} behavior the same as box.cfg{log_modules = ...} not the other way around, i.e. reset the configuration to the new one, not merge it with the existing config.

Rationale:

  • This would be consistent with other box.cfg options taking tables (listen, replication, wal_ext, audit_spaces, metrics).
  • Config table merging may be done at the upper level if necessary (in the config registry) so it's better to keep box.cfg simple.
  • If we merge the new log_modules table with the old one in box.cfg then to reset the config, the user would either have to call box.cfg twice (first box.cfg{log_modules = box.NULL} then box.cfg{log_modules = ...}) or read the box.cfg table and then set all its values to box.NULL. Both ways aren't very convenient.

@locker
Copy link
Member

locker commented Oct 20, 2023

The current behavior of log.cfg (merging the new modules table with the old one) isn't explicitly documented. The documentation for log.cfg.modules refers to box.cfg.log_modules and all code snippets use box.cfg.log_modules. So I think we can consider the current behavior of log.cfg a bug and fix it both in 2.11 and 3.0.

@locker locker changed the title Per-module logging follow-ups log.cfg{modules = ...} isn't equivalent to box.cfg{log_modules = ...} Oct 20, 2023
@locker locker added bug Something isn't working 2.11 Target is 2.11 and all newer release/master branches config and removed 3.0 Target is 3.0 and all newer release/master branches labels Oct 20, 2023
locker added a commit to locker/tarantool that referenced this issue Oct 23, 2023
Configuring log modules work differently with log.cfg and box.cfg:
box.cfg{log_modules=...} overwrites the current config completely while
log.cfg{modules=...} overwrites the currently config only for the
specified modules. Let's fix this inconsistency by making log.cfg behave
exactly as box.cfg.

Closes tarantool#7962

NO_DOC=bug fix
locker added a commit to locker/tarantool that referenced this issue Oct 23, 2023
Configuring log modules work differently with log.cfg and box.cfg:
box.cfg{log_modules=...} overwrites the current config completely while
log.cfg{modules=...} overwrites the currently config only for the
specified modules. Let's fix this inconsistency by making log.cfg behave
exactly as box.cfg.

Closes tarantool#7962

NO_DOC=bug fix
locker added a commit to locker/tarantool that referenced this issue Oct 23, 2023
Configuring log modules work differently with log.cfg and box.cfg:
box.cfg{log_modules=...} overwrites the current config completely while
log.cfg{modules=...} overwrites the currently config only for the
specified modules. Let's fix this inconsistency by making log.cfg behave
exactly as box.cfg.

Closes tarantool#7962

NO_DOC=bug fix
locker added a commit that referenced this issue Oct 24, 2023
Configuring log modules work differently with log.cfg and box.cfg:
box.cfg{log_modules=...} overwrites the current config completely while
log.cfg{modules=...} overwrites the currently config only for the
specified modules. Let's fix this inconsistency by making log.cfg behave
exactly as box.cfg.

Closes #7962

NO_DOC=bug fix
locker added a commit that referenced this issue Oct 24, 2023
Configuring log modules work differently with log.cfg and box.cfg:
box.cfg{log_modules=...} overwrites the current config completely while
log.cfg{modules=...} overwrites the currently config only for the
specified modules. Let's fix this inconsistency by making log.cfg behave
exactly as box.cfg.

Closes #7962

NO_DOC=bug fix

(cherry picked from commit c13e59a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.11 Target is 2.11 and all newer release/master branches bug Something isn't working config logger Issues related to logging subsystem
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants