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

manager, config: watch and apply the latest log configuration online #95

Merged
merged 8 commits into from
Sep 26, 2022

Conversation

djshow832
Copy link
Collaborator

@djshow832 djshow832 commented Sep 22, 2022

What problem does this PR solve?

Issue Number: ref #94

Problem Summary:
Log configurations need to take effect dynamically.

What is changed and how it works:

  • Watch the update of the log configuration and apply them to the LoggerManager.
  • Manage the LoggerManager by Server and remove the logs in main().

TODO:
The LoggerManager relies on the ConfigManager to get the log path, but ConfigManager relies on LoggerManager to get the logger. Currently, LoggerManager logs to the path specified in the static config first and then switches to the path specified in the dynamic config. There are some ways to solve it:

  • Forbid overwriting the log path
  • Do not log in the ConfigManager before the logger is initialized
  • There should be a global log path and a log path for each tenant. The global log path cannot be updated, but tenants can update theirs. Logging into different paths helps privilege control among tenants.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code
$ echo '{"log-file": {"filename":"proxy.log"}}' | ./bin/tiproxyctl config log set
""
$ ./bin/tiproxyctl config log get
{"log-file":{"filename":"proxy.log"}}
$ cat proxy.log
[2022/09/23 10:27:05.069 +08:00] [INFO] [main.gin] [/api/admin/config/log] [status=200] [method=GET] [path=/api/admin/config/log] [query=] [ip=::1] [user-agent=Go-http-client/1.1] [latency=93.715µs]

Notable changes

  • Has configuration change
  • Has HTTP API interfaces change (Don't forget to add the declarative for API)
  • Has weirctl change
  • Other user behavior changes

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

@djshow832 djshow832 requested a review from xhebox September 22, 2022 07:06
Copy link
Collaborator

@xhebox xhebox left a comment

Choose a reason for hiding this comment

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

Rest LGTM

pkg/manager/logger/manager.go Outdated Show resolved Hide resolved
lib/config/proxy.go Outdated Show resolved Hide resolved
lib/cli/config.go Outdated Show resolved Hide resolved
pkg/manager/config/proxy.go Outdated Show resolved Hide resolved
pkg/server/api/config.go Outdated Show resolved Hide resolved
pkg/manager/config/proxy.go Outdated Show resolved Hide resolved
pkg/manager/config/proxy.go Outdated Show resolved Hide resolved
pkg/manager/config/proxy.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@xhebox xhebox left a comment

Choose a reason for hiding this comment

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

Rest LGTM

lib/config/proxy.go Outdated Show resolved Hide resolved
@djshow832 djshow832 merged commit 3bf1000 into pingcap:main Sep 26, 2022
@djshow832 djshow832 deleted the push_log branch September 26, 2022 07:52
@aytrack aytrack mentioned this pull request Sep 30, 2022
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.

2 participants